More STV Cleanup #607
Merged
More STV Cleanup #607
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…leanup. Since it didn't stop the build, we are likely not using this... so...
…with other options, as it is always false.
…een used outside of Stv R&D. So keeping this for now.
klendathu2k
requested review from
genevb,
plexoos,
fisyak,
R-Witt,
iraklic,
perevbnlgov,
akioogawa,
dmarkh and
fgeurts
as code owners
plexoos
approved these changes
Oct 18, 2023
iraklic
approved these changes
Oct 18, 2023
|
Hi Irakli,
Yes and no. Yes, this is part of a general cleanup of Stv. Yes, if
there
is anything left it will not break things. No, there is not a zero risk
of
this breaking code. In particular the changes to StBFChain.
There were several places where the presence (or lack-thereof) of an Stv
chain
option altered the control flow. Many of them were logical OR's, which
were
easy to refactor...
if ( A || B || GetOption("Stv") ) { ... }
A few were slightly more complex. I likely got those right, but it
would be good
to have someone carefully review the changes to StBFChain.
Cheers,
Jason
There were several instances in StBFChain where the
presence of the Stv option, the StvMaker, etc... was checked in logic.
I believe
that I have
…On 2023-10-18 14:18, Irakli Chakaberia wrote:
@iraklic approved this pull request.
As I understand this is just a cleanup of Stv, and I assume this is
full cleanup, I have not checked if there is Svt in any other option
before the approval. But I also assume even if there is leftover it
won't brake anything anyhow.
--
Reply to this email directly, view it on GitHub [1], or unsubscribe
[2].
You are receiving this because you authored the thread.Message ID:
***@***.***>
Links:
------
[1]
#607 (review)
[2]
https://github.com/notifications/unsubscribe-auth/ANL4LVDDLBX7DN3ZBJFWG3DYAAMPTAVCNFSM6AAAAAA6ELLFHGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOBVHAYTKNRRGM
|
|
Thanks for confirming what I thought. I can try to go over the options and land you a second pair of eyes. I will let you know if I find something but let's keep the "approval" :) |
|
I think this is ready to squash/merge... but would like to give @genevb the opportunity to review the BigFullChain.h and StBFChain.cxx changes first... |
genevb
approved these changes
Oct 24, 2023
dkapukchyan
pushed a commit
to dkapukchyan/star-sw
that referenced
this pull request
Mar 11, 2024
Creating a draft PR. Additional Stv cleanup: Removes IDL files. Removes chain options, and the configuration logic in StBFChain... (note: broken out over several commits as I refactored the slightly complicated control flow...) Removed the StXTrakMaker. This depends on StvUtil. StvUtil was removed, but... our code still compiles. And I don't see any other xtrak dependencies... so draft PR should reveal whether it is a problem or not.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Creating a draft PR. Additional Stv cleanup:
Removes IDL files.
Removes chain options, and the configuration logic in StBFChain...
(note: broken out over several commits as I refactored the slightly complicated control flow...)
Removed the StXTrakMaker. This depends on StvUtil. StvUtil was removed, but... our code still compiles. And I don't see any other xtrak dependencies... so draft PR should reveal whether it is a problem or not.