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

Fix linter complaints in the validateStringAnnotations pass and enable -werror for MacOS. #4881

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Aug 22, 2024

No description provided.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Aug 22, 2024
@fruffy fruffy requested a review from vlstill August 22, 2024 08:18
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Thanks for this. I should not have copy-pasted another validator, sorry. How come the CI did not catch this? Or are you running a different linter somewhere?

@fruffy
Copy link
Collaborator Author

fruffy commented Aug 22, 2024

How come the CI did not catch this?

The sanitizer only runs nightly and uses -werror. https://github.com/p4lang/p4c/actions/runs/10499346080/job/29085947562. The other issues are just clang-tidy complaints, which are optional.

We can enable -werror on all other runs, I do not see any reason why not.

Edit: We are using -werror but the sanitizer is the only clang build.

@fruffy fruffy enabled auto-merge August 22, 2024 08:39
@vlstill
Copy link
Contributor

vlstill commented Aug 22, 2024

We can enable werror on all other runs, I do not see any reason why not.

Edit: We are using werror but the sanitizer is the only clang build.

I see, it makes sense that clang finds different bugs. It would be good to have clang build in the normal suite, but I'm not sure we have capacity for it. Do the macOS builds have Werror? I'm not sure if apple clang provides the same errors, but that might be worth the try if it is not already enabled there (and if it does not produce too many apple-specific warnings).

@fruffy fruffy disabled auto-merge August 22, 2024 08:47
@fruffy fruffy changed the title Fix linter complaints in the validateStringAnnotations pass. Fix linter complaints in the validateStringAnnotations pass and enable werror for MacOS. Aug 22, 2024
@fruffy fruffy changed the title Fix linter complaints in the validateStringAnnotations pass and enable werror for MacOS. Fix linter complaints in the validateStringAnnotations pass and enable -werror for MacOS. Aug 22, 2024
@fruffy fruffy added the run-sanitizer Use this tag to run a Clang+Sanitzers CI run. label Aug 22, 2024
@fruffy fruffy added this pull request to the merge queue Aug 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 22, 2024
Signed-off-by: fruffy <fruffy@nyu.edu>
@fruffy fruffy enabled auto-merge August 22, 2024 17:19
@fruffy fruffy added this pull request to the merge queue Aug 22, 2024
Merged via the queue into main with commit 30444b0 Aug 22, 2024
18 checks passed
@fruffy fruffy deleted the fruffy/sanitizer_fix branch August 22, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) run-sanitizer Use this tag to run a Clang+Sanitzers CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants