This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Reword -Wrange-loop-analysis warning messages
ClosedPublic

Authored by aaronpuchert on Mar 4 2020, 8:10 AM.

Details

Summary

The messages for two of the warnings are misleading:

  • warn_for_range_const_reference_copy suggests that the initialization of the loop variable results in a copy. But that's not always true, we just know that some conversion happens, potentially invoking a constructor or conversion operator. The constructor might copy, as in the example that lead to this message [1], but it might also not. However, the constructed object is bound to a reference, which is potentially misleading, so we rewrite the message to emphasize that. We also make sure that we print the reference type into the warning message to clarify that this warning only appears when operator* returns a reference.
  • warn_for_range_variable_always_copy suggests that a reference type loop variable initialized from a temporary "is always a copy". But we don't know this, the range might just return temporary objects which aren't copies of anything. (Assuming RVO a copy constructor might never have been called.)

The message for warn_for_range_copy is a bit repetitive: the type of a
VarDecl and its initialization Expr are the same up to cv-qualifiers,
because Sema will insert implicit casts or constructor calls to make
them match.

[1] https://bugs.llvm.org/show_bug.cgi?id=32823

Diff Detail

Event Timeline

aaronpuchert created this revision.Mar 4 2020, 8:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2020, 8:10 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaronpuchert added a comment.EditedMar 4 2020, 8:16 AM

This addresses my earlier comment D73007#1853563.

Thanks for the patch! I've some minor nits, but other then that it looks fine.

clang/include/clang/Basic/DiagnosticSemaKinds.td
2427

I'd like to change this name to reflect the changed diagnostic.

2434

I'd like to change this name to reflect the changed diagnostic.

  • Change diagnostic names to reflect the changed messages.
  • Also change the wording of note_use_type_or_non_reference.
  • The warning that was warn_for_range_const_reference_copy is only emitted if the operator* call returns a reference, so we should also print a reference type in the warning message. Otherwise this would be confusing.
aaronpuchert edited the summary of this revision. (Show Details)Mar 4 2020, 2:46 PM
aaronpuchert marked 2 inline comments as done.

Drop SpelledAsLValue = false, which seems to be wrong.

Harbormaster completed remote builds in B48120: Diff 248332.
This revision is now accepted and ready to land.Mar 5 2020, 1:48 PM
This revision was automatically updated to reflect the committed changes.