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.

Sign up for GitHub

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

Jump to bottom

llvm/lib/CodeGen/TargetSchedule.cpp:132:12: warning: Assert statement modifies 'NIter' #90982

Merged
merged 1 commit into from
May 4, 2024
Merged

llvm/lib/CodeGen/TargetSchedule.cpp:132:12: warning: Assert statement modifies 'NIter' #90982

merged 1 commit into from
May 4, 2024

Conversation

akshaykumars614
Copy link
Contributor

Modified the assert statement

@akshaykumars614
Copy link
Contributor Author

I'm new to this project. Please let me know if this change is OK and if any tests required.

@JOE1994
Copy link
Member

JOE1994 commented May 3, 2024 •
edited
Loading

No test is needed.

Please change the title of this PR & commit message title to informative text (instead of a URL).

By writing closes #90328 in the commit message, you can make the linked issue get closed automatically once your commit gets merged to main.

@akshaykumars614 akshaykumars614 changed the title https://github.com/llvm/llvm-project/issues/90328 llvm/lib/CodeGen/TargetSchedule.cpp:132:12: warning: Assert statement modifies 'NIter' May 4, 2024
@JOE1994
Copy link
Member

JOE1994 commented May 4, 2024

Could you update the title of the commit to something like "[llvm] Move variable update out of assert (NFC)" ?
For LLVM commit message titles, it is standard practice to use headers indicating the updated code area, and then add summary of the change. "NFC" at the end means "No Functional Change".

I'd also suggest to move the "closes #90328" message from the title to the body of the commit message.

@akshaykumars614
[llvm] Move variable update out of assert (NFC)
a5506b9 
closes llvm#90328
llvm/lib/CodeGen/TargetSchedule.cpp:132:12: warning: Assert statement modifies 'NIter'
@akshaykumars614
Copy link
Contributor Author

Got it. Thanks for the suggestion :)

@akshaykumars614 akshaykumars614 merged commit 18d1df4 into llvm:main May 4, 2024
3 of 4 checks passed
@akshaykumars614 akshaykumars614 deleted the 90328 branch May 4, 2024 18:24
@dwblaikie dwblaikie mentioned this pull request May 4, 2024
Merged
dwblaikie added a commit that referenced this pull request May 4, 2024
@dwblaikie
Revert "llvm/lib/CodeGen/TargetSchedule.cpp:132:12: warning: Assert s…
0044856 
…tatement modifies 'NIter'" (#91079)

Reverts #90982

NIter was only declared in !NDEBUG, and only used for assertions - so it
was correct that it was incremented inside the assertion. (& in fact now
the non-asserts build fails, because the variable is incremented even
though it isn't declared)
sookach pushed a commit to sookach/llvm-project that referenced this pull request May 4, 2024
@akshaykumars614 @sookach
llvm/lib/CodeGen/TargetSchedule.cpp:132:12: warning: Assert statement…
e0cd693 
… modifies 'NIter' (llvm#90982)

Modified the assert statement
sookach pushed a commit to sookach/llvm-project that referenced this pull request May 4, 2024
@dwblaikie @sookach
Revert "llvm/lib/CodeGen/TargetSchedule.cpp:132:12: warning: Assert s…
80c1743 
…tatement modifies 'NIter'" (llvm#91079)

Reverts llvm#90982

NIter was only declared in !NDEBUG, and only used for assertions - so it
was correct that it was incremented inside the assertion. (& in fact now
the non-asserts build fails, because the variable is incremented even
though it isn't declared)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JOE1994 JOE1994 JOE1994 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@akshaykumars614 @JOE1994