Page MenuHomePhabricator

[SemaCXX] Fix false positive of -Wuninitialized-const-reference in empty function body.
ClosedPublic

Authored by zequanwu on Jun 23 2020, 6:13 PM.

Details

Summary

Some libraries use empty function to ignore unused variable warnings, which gets a new warning from -Wuninitialized-const-reference, discussed here https://reviews.llvm.org/D79895#2107604.
This patch should fix that.

Diff Detail

Event Timeline

zequanwu created this revision.Jun 23 2020, 6:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2020, 6:13 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hans added a comment.Jun 24 2020, 2:02 AM

Okay, since checking this is cheap I suppose we can do it.

clang/lib/Analysis/UninitializedValues.cpp
410

Is this special check for templates necessary? Doesn't the "fd->hasTrivialBody()" check below also handle the template case?

clang/test/SemaCXX/warn-uninitialized-const-reference.cpp
13

I think "const T &" would be the more common ordering. Also the "inline" isn't really necessary.

47

I would add a comment explaining that this is a pattern to avoid "unused variable" warnings, and mention the boost function as an example.

nick added a comment.Jun 24 2020, 5:14 AM

Thanks @zequanwu, much appreciated.

zequanwu updated this revision to Diff 273103.Jun 24 2020, 10:57 AM
zequanwu marked an inline comment as done.

Address comments.

zequanwu marked 2 inline comments as done.Jun 24 2020, 10:57 AM
zequanwu added inline comments.
clang/lib/Analysis/UninitializedValues.cpp
410

Yes, it is necessary. If the function is a function template, fd doesn't have body when this analysis happens. So fd->hasTrivialBody() is always false in that case.

Personally, I think this behavior is a bit mysterious given that there are *much* better ways to silence unused variable warnings (like casting to void) that have been well-supported by compilers for a long time and I'd prefer not to penalize every call expression for such an unusual code pattern. That said, it should probably not be much of a performance hit, so it may be reasonable (having compile time performance numbers pre and post patch would be nice, though).

clang/lib/Analysis/UninitializedValues.cpp
409–410

These names don't match the usual coding styles, should probably be FD and FTD.

Also, what about calls to something other than a function, like a block?

435

This can be hoisted out of the loop so that we don't have to check the same thing on every argument.

clang/test/SemaCXX/warn-uninitialized-const-reference.cpp
13

I'd also appreciate an example that explicitly shows we mean *empty* and not *trivial*:

void dont_ignore_non_empty(const int &) { ; } // Calling this won't silence the warning for you

Also, should we test a function try block? (I would consider those to never have an empty body, but I don't recall how that API reports it.)

void ignore_function_try_block_maybe_who_knows(const int &) try {} catch (...) {}

(If we add support for other callables like blocks, those should be tested as well.)

zequanwu updated this revision to Diff 273184.Jun 24 2020, 3:48 PM
zequanwu marked an inline comment as done.

Add test cases.

zequanwu marked 2 inline comments as done.Jun 24 2020, 3:52 PM
zequanwu added inline comments.
clang/lib/Analysis/UninitializedValues.cpp
435

The DeclRefExpr needs to be set to Ignore like VisitCastExpr does. Otherwise, it maybe classified to Init by isTrackedVar in ClassifyRefs::get.

nick added inline comments.Jun 24 2020, 4:26 PM
clang/lib/Analysis/UninitializedValues.cpp
435

Could not the empty body check be done in reportConstRefUse, after isUninitialized?

zequanwu marked an inline comment as done.Jul 5 2020, 12:36 PM
zequanwu added inline comments.
clang/lib/Analysis/UninitializedValues.cpp
435

No, reportConstRefUse doesn't know if the called function has trivial body or not.

aaron.ballman accepted this revision.Jul 6 2020, 7:11 AM

LGTM despite my enduring sadness that Boost elected to use this totally bizarre approach to silencing unused variable diagnostics when far better-supported options have existed for so long.

clang/lib/Analysis/UninitializedValues.cpp
435

You made the change I was looking for -- I just needed the hasTrivialBody() call hoisted since that was invariant within the loop body.

This revision is now accepted and ready to land.Jul 6 2020, 7:11 AM
This revision was automatically updated to reflect the committed changes.