Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for DL Stage #1

Merged
merged 4 commits into from
Jul 16, 2024
Merged

Add support for DL Stage #1

merged 4 commits into from
Jul 16, 2024

Conversation

eddybl
Copy link

@eddybl eddybl commented Jul 16, 2024

Source is the internal patch file

I removed the std::cout commands and also therefore #include <iostream>

This is IBPT internal review, after that I would create a PR to upstream

@eddybl eddybl added the enhancement New feature or request label Jul 16, 2024
@eddybl eddybl requested a review from smarsching July 16, 2024 08:42
@eddybl eddybl self-assigned this Jul 16, 2024
Copy link

@smarsching smarsching left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this looks good to me, I just found a few inconsistencies regarding formatting of the code and one change that looks redundant to me.

newportApp/src/AG_CONEX.cpp Outdated Show resolved Hide resolved
newportApp/src/AG_CONEX.cpp Outdated Show resolved Hide resolved
newportApp/src/AG_CONEX.cpp Outdated Show resolved Hide resolved
newportApp/src/AG_CONEX.cpp Outdated Show resolved Hide resolved
Copy link

@smarsching smarsching left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one line remaining where the indendation is still wrong. Apart from this, everything looks good to me now.

Your point about the typo in the debug code is valid. I agree that it is probably better to fix this in a separate commit, because it is unrelated to adding support for the DL stage.

I think that the whole handling of debugging isn’t very nice at the moment. It would be better to use asynPrint wherever possible and use conditional debug statements everywhere else (or, if someone is really worried about performance, wrap the debug code in #ifdefs). Just about every other solution is clearner than leaving commented out code in the sources. However, this is a completely separate issue, so I do not think that this has to or should be addressed in the context of this PR.

newportApp/src/AG_CONEX.cpp Outdated Show resolved Hide resolved
@eddybl eddybl merged commit 6348a84 into add-dl-stage Jul 16, 2024
@eddybl eddybl deleted the add-support-dl-stage branch July 16, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants