Page MenuHomePhabricator

[NFC] Fixes -Wrange-loop-analysis warnings
ClosedPublic

Authored by Mordante on Dec 24 2019, 10:59 AM.

Details

Summary

This avoids new warnings due to D68912 adds -Wrange-loop-analysis to -Wall.

This should be the last cleanup patch.

Diff Detail

Event Timeline

Mordante created this revision.Dec 24 2019, 10:59 AM

Unit tests: pass. 61116 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

xbolva00 added inline comments.Dec 31 2019, 5:39 AM
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
22

NFC patch?

It seems like some of the deductions are not spelling out the qualifiers or pointer/references explicitly in some cases, at least from my spot-checking. Can you go back through this patch to make sure we're getting those correct?

clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
78

Shouldn't this deduce to const auto *?

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
795

Same here.

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
75

auto *?

llvm/lib/Support/CommandLine.cpp
246

auto *?

2115

auto *?

Mordante updated this revision to Diff 235763.Jan 1 2020, 7:21 AM
Mordante retitled this revision from Fixes -Wrange-loop-analysis warnings to [NFC] Fixes -Wrange-loop-analysis warnings.

Reviewed the types and added a * for pointers and added a const when applicable.

Mordante marked 8 inline comments as done.Jan 1 2020, 7:23 AM
Mordante added inline comments.
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
22

Changed to NFC.

75

Changed to const auto *

aaron.ballman accepted this revision.Jan 1 2020, 7:35 AM

LGTM, thank you!

This revision is now accepted and ready to land.Jan 1 2020, 7:35 AM
Mordante marked 2 inline comments as done.Jan 1 2020, 7:39 AM

Thanks for the review! I'll commit all the -Wrange-loop-analysis patches later today.

This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.EditedJan 1 2020, 3:35 PM

I really like the new feature. Thanks for making the efforts!

However, I am afraid I don't like some of the fixes here. You can replace const auto with const auto & and call that a fix... IMHO if the type is not obvious, const ConcreteType & will be better.

I think there is a false positive.

https://github.com/llvm/llvm-project/tree/master/lld/ELF/Relocations.cpp#L1622

for (const std::pair<ThunkSection *, uint32_t> ts : isd->thunkSections)

Diagnostic:

lld/ELF/Relocations.cpp:1622:14: note: use reference type 'const std::pair<ThunkSection *, uint32_t> &' (aka 'const pair<lld::elf::ThunkSection *, unsigned int> &') to prevent copying
            for (const std::pair<ThunkSection *, uint32_t> ts : isd->thunkSections)

i.e. The diagnostic asks me to replace const std::pair<T *, size_t> with const std::pair<T *, size_t> &, when it is clear that the type is cheap to copy.

However, I am afraid I don't like some of the fixes here. You can replace const auto with const auto & and call that a fix... IMHO if the type is not obvious, const ConcreteType & will be better.

I notice some parts of the code prefer auto and others ConcreteType, so there is no consensus on what is the best. I feel with this change I want to change as little as possible.

I think there is a false positive.

https://github.com/llvm/llvm-project/tree/master/lld/ELF/Relocations.cpp#L1622

for (const std::pair<ThunkSection *, uint32_t> ts : isd->thunkSections)

Diagnostic:

lld/ELF/Relocations.cpp:1622:14: note: use reference type 'const std::pair<ThunkSection *, uint32_t> &' (aka 'const pair<lld::elf::ThunkSection *, unsigned int> &') to prevent copying
            for (const std::pair<ThunkSection *, uint32_t> ts : isd->thunkSections)

i.e. The diagnostic asks me to replace const std::pair<T *, size_t> with const std::pair<T *, size_t> &, when it is clear that the type is cheap to copy.

The code has a 'protection' for this case clang/lib/Sema/SemaStmt.cpp:2803:

// TODO: Determine a maximum size that a POD type can be before a diagnostic
// should be emitted.  Also, only ignore POD types with trivial copy
// constructors.
if (VariableType.isPODType(SemaRef.Context))
  return;

I could change this protection to test whether the type is trivially copyable which will reduce the number of warnings. Unfortunately std::pair is not trivially copyable so that won't help here. Do you think it makes sense to allow trivially copyable types instead of POD types? Any suggestion how we could solve it for std::pair?

I think there is a false positive.

https://github.com/llvm/llvm-project/tree/master/lld/ELF/Relocations.cpp#L1622

for (const std::pair<ThunkSection *, uint32_t> ts : isd->thunkSections)

Removing the const like L1648 will also solve the issue.

I think there is a false positive.

https://github.com/llvm/llvm-project/tree/master/lld/ELF/Relocations.cpp#L1622

for (const std::pair<ThunkSection *, uint32_t> ts : isd->thunkSections)

Removing the const like L1648 will also solve the issue.

Yes, both std::pair and auto work, but const std::pair doesn't. std::pair has an unfortunate user-defined copy assignment operator (https://eel.is/c++draft/pairs#pair-16 "Assigns p.first to first and p.second to second." Does this wording mandate a user-defined function?) I will drop const for this lld file.

For other user-defined types. I think loosening the diagnostic to allow trivially copyable types will be nice. Types that take only a few words (say. 1~3) are likely faster with a copy.