This avoids new warnings due to D68912 adds -Wrange-loop-analysis to -Wall.
This should be the last cleanup patch.
Paths
| Differential D71857
[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 TimelineHerald added subscribers: bmahjour, usaxena95, kadircet and 7 others. · View Herald TranscriptDec 24 2019, 10:59 AM Comment Actions 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
Comment Actions 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?
Mordante retitled this revision from Fixes -Wrange-loop-analysis warnings to [NFC] Fixes -Wrange-loop-analysis warnings. Comment ActionsReviewed the types and added a * for pointers and added a const when applicable. Mordante added inline comments. This revision is now accepted and ready to land.Jan 1 2020, 7:35 AM Comment Actions Thanks for the review! I'll commit all the -Wrange-loop-analysis patches later today. Closed by commit rG8dc7b982b455: [NFC] Fixes -Wrange-loop-analysis warnings (authored by Mordante). · Explain WhyJan 1 2020, 11:07 AM This revision was automatically updated to reflect the committed changes. Comment Actions 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. Comment Actions
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.
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? Comment Actions
Removing the const like L1648 will also solve the issue. Comment Actions
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.
Revision Contents
Diff 235780 clang-tools-extra/clang-doc/MDGenerator.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
clang-tools-extra/clangd/index/MemIndex.cpp
clang/lib/CodeGen/CodeGenPGO.cpp
clang/lib/CodeGen/ItaniumCXXABI.cpp
clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
clang/lib/Tooling/ASTDiff/ASTDiff.cpp
clang/tools/clang-refactor/TestSupport.cpp
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
lldb/source/Plugins/Platform/Android/AdbClient.cpp
lldb/source/Target/StackFrameRecognizer.cpp
llvm/include/llvm/Analysis/LoopInfo.h
llvm/include/llvm/Analysis/LoopInfoImpl.h
llvm/include/llvm/Support/GenericDomTree.h
llvm/lib/Analysis/DomTreeUpdater.cpp
llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
llvm/lib/Analysis/ScalarEvolution.cpp
llvm/lib/CodeGen/InlineSpiller.cpp
llvm/lib/CodeGen/InterleavedLoadCombinePass.cpp
llvm/lib/CodeGen/RegAllocFast.cpp
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
llvm/lib/DebugInfo/Symbolize/Symbolize.cpp
llvm/lib/ExecutionEngine/Orc/ExecutionUtils.cpp
llvm/lib/IR/TypeFinder.cpp
llvm/lib/Linker/IRMover.cpp
llvm/lib/MC/XCOFFObjectWriter.cpp
llvm/lib/MCA/HardwareUnits/ResourceManager.cpp
llvm/lib/MCA/Stages/InstructionTables.cpp
llvm/lib/ObjectYAML/CodeViewYAMLDebugSections.cpp
llvm/lib/Support/CommandLine.cpp
llvm/lib/Support/TargetParser.cpp
|
Shouldn't this deduce to const auto *?