This is an archive of the discontinued LLVM Phabricator instance.

ISSUE - incorrect -Winfinite-recursion warning on potentially-unevaluated operand #21668
ClosedPublic

Authored by appmonster007 on Jun 28 2022, 11:24 AM.

Details

Summary

Fixing issue "incorrect -Winfinite-recursion warning on potentially-unevaluated operand #21668”

By having dedicated visit function (VisitCXXTypeidExpr) for typeid, instead of using default (VisitStmt).
In this new function we skip over CFG build for unevaluated operands of typeid

Diff Detail

Event Timeline

appmonster007 created this revision.Jun 28 2022, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2022, 11:24 AM
appmonster007 requested review of this revision.Jun 28 2022, 11:24 AM
shafik added a subscriber: shafik.Jun 28 2022, 2:45 PM

Thank for looking at this bug, just a small comment on the test.

clang/test/SemaCXX/warn-infinite-recursion.cpp
178

Looks like your missing // expected-warning{{call itself}}

fixed warning comments for test cases

appmonster007 marked an inline comment as done.Jun 28 2022, 6:19 PM

included typeinfo file, in clang/test/SemaCXX directory

#include "typeinfo"

appmonster007 added inline comments.Jun 28 2022, 9:29 PM
clang/test/SemaCXX/warn-infinite-recursion.cpp
3

#include <typeinfo> resulted in 'typeinfo' file not found with <angled> include; use "quotes" instead

Thank you for this fix, it's coming along well! Can you also add a release note to clang/docs/ReleaseNotes.rst mentioning the fix?

clang/lib/Analysis/CFG.cpp
4062
4064–4068
clang/test/SemaCXX/typeinfo
2–17

You should stick this into the place where it's used rather than a separate file, more comments below about why.

clang/test/SemaCXX/warn-infinite-recursion.cpp
3

Yup, that's actually by design. We don't want to include headers from the system because then we're testing against whatever happens to be installed on the machine running the test, which makes for poor test coverage. What we usually do is put the system header definitions directly into the file under test, or if it's code that's going to be shared by multiple tests, we'll make a file for it in the Inputs directory, and tell the RUN line to look there when doing an include. Then we always know exactly what we're testing against.

In this case, I'd just lower the type_info class into this file.

178
191

Please add a newline back to the end of the file.

inserted bullet point for -Winfinite-recursion diagnostics fix, where diagnostic will not warn for unevaluated operands of `typeid` expression, under "Improvements to Clang’s diagnostics" in clang/docs/ReleaseNotes.rst

fixed issue with typeinfo header, just included it directly in warn-infinite-recursion.cpp file

fixed recommended minor changes

appmonster007 marked an inline comment as done.
appmonster007 marked 4 inline comments as done.Jun 29 2022, 7:16 AM
aaron.ballman accepted this revision.Jun 29 2022, 10:15 AM

Thank you for the fix, the changes LGTM! From our off-review discussion, I understand you need me to commit this on your behalf. What name and email address would you like me to use for patch attribution? I did have a suggestion on changing the release notes slightly, but I can do that for you when I land the changes, so don't feel obligated to push up a new patch unless you'd like to.

clang/docs/ReleaseNotes.rst
278–281 ↗(On Diff #441014)

Reworded a bit.

This revision is now accepted and ready to land.Jun 29 2022, 10:15 AM

Name : Prathit Aswar
github username : appmonster007
email : snaswar0201@gmail.com

appmonster007 marked an inline comment as done.Jun 29 2022, 10:11 PM
NoQ added a subscriber: usama54321.Jun 30 2022, 8:07 PM

Thanks, very nice!

@usama54321 you recently fixed a similar problem in a clang-tidy check, do you know if it also requires a similar clause for polymorphic expressions inside typeid?