This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix wrong -Wunused-local-typedef warning within a template function
ClosedPublic

Authored by krisb on Nov 22 2021, 10:32 AM.

Details

Summary

Partially fixes PR24883.

The patch sets Reference bit while instantiating a typedef if it
previously was found referenced.

Diff Detail

Event Timeline

krisb requested review of this revision.Nov 22 2021, 10:32 AM
krisb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2021, 10:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
krisb added inline comments.Nov 22 2021, 10:33 AM
clang/test/Modules/odr_hash.cpp
4288

I'm not sure what was the original intent of this test (i.e. whether it intentionally tests the fact that there is no error on an uninstantiated static member function). As well as it doesn't clear to me what is the role of the unused typedef here, but it starts triggering the error because of redecls() call on isReferenced() added by this patch.

rtrieu added inline comments.Dec 1 2021, 2:15 AM
clang/test/Modules/odr_hash.cpp
4288

I've checked out the ODR Hashing versus the other cases. The new error is in line with the other behavior. Having this new error is okay.

krisb added a comment.Dec 10 2021, 2:47 AM

@rtrieu, thank you for looking at this! Do you have any comments about the rest of the patch? Do you think it makes sense?

clang/test/Modules/odr_hash.cpp
4288

Thank you for checking this!

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 2:06 AM
thakis accepted this revision.Mar 7 2022, 8:21 AM

Seems like a progression :)

Do you have commit permissoins?

This revision is now accepted and ready to land.Mar 7 2022, 8:21 AM
Quuxplusone added inline comments.
clang/test/SemaCXX/warn-unused-local-typedef.cpp
246

I haven't tried to understand the main point of this PR, but FWIW, have you seen https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61596 ? This looks suspiciously like that exact case. (I just filed its dup https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104792 the other day.)

krisb added inline comments.Mar 7 2022, 4:05 PM
clang/test/SemaCXX/warn-unused-local-typedef.cpp
246

This patch prevents clang from warning on used typedefs (Int case), if those typedefs are defined within a local class of a templated function or a member function of a templated class.
For Char case, which is unused in this example, the patch changes nothing.

Regarding https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104792, those typedefs seem well known, so could we just make clang skipping any checks for those particular names?
Alternatively, we would likely need to stop warning on any typedefs defined within a function-local class (but those warnings may still be useful like in the test from this patch).

krisb added inline comments.Mar 14 2022, 11:48 AM
clang/test/SemaCXX/warn-unused-local-typedef.cpp
246

@Quuxplusone if you do not have objections for this patch, I'd land it now. It'll make things a bit better and do not prevent us from relaxing conditions for the warning further.

Unknown Object (User) added a subscriber: Unknown Object (User).Oct 19 2022, 6:45 PM

thanks for your post! It helps me so much!
five nights at freddy's