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

[analyzer] Use -imacros flag instead of -macros #3428

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

whisperity
Copy link
Contributor

This was seemingly last touched in #2449 (7d93b32) but it seems like a typo. There's no such flag as -macros in neither Clang nor GCC.

via @steakhal

This was seemingly last touched in Ericsson#2449
(Ericsson/codechecker@7d93b32) but it
seems like a typo. There's no such flag as `-macros` in neither Clang
nor GCC.
@whisperity whisperity requested a review from bruntib September 8, 2021 13:31
@csordasmarton csordasmarton added this to the release 6.17.0 milestone Sep 9, 2021
@csordasmarton csordasmarton added analyzer 📈 Related to the analyze commands (analysis driver) config ⚙️ labels Sep 9, 2021
Copy link
Contributor

@csordasmarton csordasmarton left a comment

Choose a reason for hiding this comment

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

LGTM!

@bruntib
Copy link
Contributor

bruntib commented Sep 9, 2021

I wouldn't be so brave to do this modification in the last minute before the release. It is possible that we met this flag in a non-standard compiler. If the addition of -imacros is needed, then add it instead of change the previous one.

@whisperity
Copy link
Contributor Author

If the addition of -imacros is needed, then add it instead of change the previous one.

@bruntib Yeah that's why I wanted you to look at the patch too... I couldn't figure out the reasoning behind the changes in the previous patch.

The addition of -imacros is definitely needed, the patch is coming from field experience with a user whose build broke specifically because of this argument (their build system used it like 3 times) was torn out from the analysis invocation.

I'll change the patch ASAP!

@csordasmarton
Copy link
Contributor

I think it was a typo during code refactoring when this macros flag was skipped instead of imacros in the following commit: 1a58634

Previous imacros was used:
macros

@steakhal
Copy link
Contributor

steakhal commented Sep 9, 2021

I think it was a typo during code refactoring when this macros flag was skipped instead of imacros in the following commit: 1a58634

Previous imacros was used:
macros

This seems like a clear indication of a typo. IMO the patch is good as-is.

@whisperity
Copy link
Contributor Author

(Side note: we should really put some comments up what these big lists of flags mean.)

I have the updated patch ready if you want that, but I am fine with keeping the current patch.

@steakhal
Copy link
Contributor

I guess this missed the 6.17.0 release.

@whisperity
Copy link
Contributor Author

whisperity commented Sep 13, 2021

I guess this missed the 6.17.0 release.

@steakhal Yeah, seems like so, I just asked. We could target both 6.17.1 and 6.18 though.

@csordasmarton csordasmarton modified the milestones: release 6.17.0, release 6.17.1 Sep 15, 2021
@bruntib bruntib merged commit 08fc9c0 into Ericsson:master Sep 17, 2021
@csordasmarton csordasmarton modified the milestones: release 6.17.1, release 6.18.0 Nov 4, 2021
@whisperity whisperity deleted the imacros-ignore branch April 11, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer 📈 Related to the analyze commands (analysis driver) config ⚙️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants