Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Nico Weber via cfe-commits]

Also, we only use auto if the type of the variable is clear. Changes like

-  for (ModuleFile  : llvm::reverse(ModuleMgr)) {
+  for (auto  : llvm::reverse(ModuleMgr)) {

are not desired.

On Sat, Apr 14, 2018, 11:09 AM Malcolm Parsons via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> On Sat, 14 Apr 2018, 14:16 Kim Gräsman,  wrote:
>
>> That would be a nice outcome of all the "run-tools-on-llvm" changes if
>> any problems were filed as bugs on the tools. We have a number of them
>> filed on iwyu, and they make for nice, concrete bugs to troubleshoot even
>> if we don't always know how to fix them.
>>
>> For this specific clang-tidy issue, do you have any ideas for how to tell
>> this loop apart from any other? I'm guessing the container is modified
>> while iterating... Or do you mean skip all non-iterator loops?
>>
>
> Non-iterator, mutable container, size checked each iteration.
>
> Clang-tidy could suggest modernisation, but not automatically fix.
>
> --
> Malcolm Parsons
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Malcolm Parsons via cfe-commits]

On Sat, 14 Apr 2018, 14:16 Kim Gräsman,  wrote:

> That would be a nice outcome of all the "run-tools-on-llvm" changes if any
> problems were filed as bugs on the tools. We have a number of them filed on
> iwyu, and they make for nice, concrete bugs to troubleshoot even if we
> don't always know how to fix them.
>
> For this specific clang-tidy issue, do you have any ideas for how to tell
> this loop apart from any other? I'm guessing the container is modified
> while iterating... Or do you mean skip all non-iterator loops?
>

Non-iterator, mutable container, size checked each iteration.

Clang-tidy could suggest modernisation, but not automatically fix.

-- 
Malcolm Parsons
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Kim Gräsman via cfe-commits]

That would be a nice outcome of all the "run-tools-on-llvm" changes if any
problems were filed as bugs on the tools. We have a number of them filed on
iwyu, and they make for nice, concrete bugs to troubleshoot even if we
don't always know how to fix them.

For this specific clang-tidy issue, do you have any ideas for how to tell
this loop apart from any other? I'm guessing the container is modified
while iterating... Or do you mean skip all non-iterator loops?

- Kim

Den lör 14 apr. 2018 12:20Malcolm Parsons via cfe-commits <
cfe-commits@lists.llvm.org> skrev:

> On Sat, 14 Apr 2018, 04:22 Richard Trieu via cfe-commits, <
> cfe-commits@lists.llvm.org> wrote:
>
>> I was tracking down a similar issue to the lldb issue before noticing the
>> change was reverted.  The bad change that lead to it is:
>>
>>  // Load pending declaration chains.
>> -for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
>> -  loadPendingDeclChain(PendingDeclChains[I].first,
>> PendingDeclChains[I].second);
>> +for (const auto  : PendingDeclChains)
>> +  loadPendingDeclChain(I.first, I.second);
>>  PendingDeclChains.clear();
>>
>> Although the two looks like similar, the vector PendingDeclChains is a
>> class member and gets new elements during loop runs.  Once enough elements
>> are added to the vector, it get reallocated to a larger memory, but the
>> loop is still trying to process the old, now freed, memory.  Using an index
>> and checking the size every loop is the right way to process this vector.
>>
>
> Should clang-tidy handle this type of loop differently?
>
> --
> Malcolm Parsons
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Malcolm Parsons via cfe-commits]

On Sat, 14 Apr 2018, 04:22 Richard Trieu via cfe-commits, <
cfe-commits@lists.llvm.org> wrote:

> I was tracking down a similar issue to the lldb issue before noticing the
> change was reverted.  The bad change that lead to it is:
>
>  // Load pending declaration chains.
> -for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
> -  loadPendingDeclChain(PendingDeclChains[I].first,
> PendingDeclChains[I].second);
> +for (const auto  : PendingDeclChains)
> +  loadPendingDeclChain(I.first, I.second);
>  PendingDeclChains.clear();
>
> Although the two looks like similar, the vector PendingDeclChains is a
> class member and gets new elements during loop runs.  Once enough elements
> are added to the vector, it get reallocated to a larger memory, but the
> loop is still trying to process the old, now freed, memory.  Using an index
> and checking the size every loop is the right way to process this vector.
>

Should clang-tidy handle this type of loop differently?

-- 
Malcolm Parsons
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Vedant Kumar via cfe-commits]

I've gone ahead and reverted this in r330080.

I tested this with "./bin/lldb-dotest -p TestForwardDecl", and it no longer 
asserts, etc.

@Eugene, regarding the specifics of this commit, there are a number of 
refactors here that don't make sense. E.g there's no need to create a reference 
to a StringRef, it's already an immutable reference :).

Stepping back a bit, I'm asking you to please stop committing large refactors 
of code you don't have a deep familiarity with, or plans to work on. I've asked 
you this before, when changes you made in lib/ProfileData caused downstream 
breakage. I'm repeating the request now. These sorts of changes carry more risk 
than I'm comfortable with, without offering up much reward in return. Please, 
enough.

thanks,
vedant

> On Apr 13, 2018, at 6:42 PM, Davide Italiano  wrote:
> 
> The amount of churn this patches introduces is very large for a very small 
> gain.
> Other than basically causing to every downstream consumer with private clang 
> changes a large amount of pain, it also introduced a bug (which shows up only 
> on lldb bots).
> I really think we should have a policy that these commits shouldn’t be 
> encouraged. At some point I and Chandler discussed this on IRC, but I’m not 
> sure this went anywhere.
> 
> In general, large refactoring of code not properly understood is causing more 
> harm to the project than good. We’re going to revert this now, and I would 
> like to kick off a discussion about this in the future.
> 
> Thanks,
> 
> —
> Davide
> 
>> On Apr 13, 2018, at 18:36, Vedant Kumar  wrote:
>> 
>> + Davide
>> 
>>> On Apr 13, 2018, at 6:36 PM, Vedant Kumar via cfe-commits 
>>>  wrote:
>>> 
>>> This is breaking the lldb bots with assertion failures:
>>> 
>>> http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/
>>> http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/6341/
>>> 
>>> stderr: Assertion failed: (M && "imported decl from no module file"), 
>>> function loadPendingDeclChain, file 
>>> /Users/vsk/src/llvm.org-lldbsan/llvm/tools/clang/lib/Serialization/ASTReaderDecl.cpp,
>>>  line 3861.
>>> Stack dump:
>>> 0.  Program arguments: 
>>> /Users/vsk/src/builds/llvm.org-lldbsan-RA/bin/clang-7 -cc1 -triple 
>>> x86_64-apple-macosx10.13.0 -Wdeprecated-objc-isa-usage 
>>> -Werror=deprecated-objc-isa-usage -emit-obj -mrelax-all -disable-free 
>>> -main-file-name main.m -mrelocation-model pic -pic-level 2 -mthread-model 
>>> posix -mdisable-fp-elim -masm-verbose -munwind-tables -target-cpu penryn 
>>> -dwarf-column-info -dwarf-ext-refs -fmodule-format=obj 
>>> -debug-info-kind=standalone -dwarf-version=4 -debugger-tuning=lldb 
>>> -target-linker-version 405.15 -coverage-notes-file 
>>> /Users/vsk/src/builds/llvm.org-lldbsan-RA/lldb-test-build.noindex/lang/objc/forward-decl/TestForwardDecl.test_expr_gmodules/main.gcno
>>>  -resource-dir /Users/vsk/src/builds/llvm.org-lldbsan-RA/lib/clang/7.0.0 
>>> -include 
>>> /Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lang/objc/forward-decl/../../../make/test_common.h
>>>  -isysroot 
>>> /Volumes/Xcode10A144_m18A232_i16A233_t16J228_w16R229_XcodeInternals_ASan_30GB/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk
>>>  -I 
>>> /Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lang/objc/forward-decl/../../../make/../../../../../include
>>>  -I 
>>> /Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lang/objc/forward-decl/
>>>  -I 
>>> /Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lang/objc/forward-decl/../../../make/
>>>  -O0 -fdebug-compilation-dir 
>>> /Users/vsk/src/builds/llvm.org-lldbsan-RA/lldb-test-build.noindex/lang/objc/forward-decl/TestForwardDecl.test_expr_gmodules
>>>  -ferror-limit 19 -fmessage-length 0 -stack-protector 1 -fno-builtin 
>>> -fblocks -fencode-extended-block-signature -fmodules -fimplicit-module-maps 
>>> -fmodules-cache-path=module-cache -fobjc-runtime=macosx-10.13.0 
>>> -fobjc-exceptions -fexceptions -fmax-type-align=16 
>>> -fdiagnostics-show-option -o main.o -x objective-c 
>>> /Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lang/objc/forward-decl/main.m
>>>  
>>> 1.  
>>> /Users/vsk/src/llvm.org-lldbsan/llvm/tools/lldb/packages/Python/lldbsuite/test/lang/objc/forward-decl/Container.h:10:10:
>>>  current parser token ';'
>>> 0  clang-7  0x00010f38bf18 
>>> llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 40
>>> 1  clang-7  0x00010f38c5a6 SignalHandler(int) + 422
>>> 2  libsystem_platform.dylib 0x7fff72cc6f5a _sigtramp + 26
>>> 3  libsystem_platform.dylib 00 _sigtramp + 2368966848
>>> 4  libsystem_c.dylib0x7fff72af1312 abort + 127
>>> 5  libsystem_c.dylib0x7fff72ab9368 basename_r + 0
>>> 6  clang-7  0x00011005cfe1 
>>> 

r330068 - [Serialization] Fix some Clang-tidy modernize and Include What You Use warnings; other minor fixes (NFC).

[Eugene Zelenko via cfe-commits]

Author: eugenezelenko
Date: Fri Apr 13 14:12:33 2018
New Revision: 330068

URL: http://llvm.org/viewvc/llvm-project?rev=330068=rev
Log:
[Serialization] Fix some Clang-tidy modernize and Include What You Use 
warnings; other minor fixes (NFC).

Modified:
cfe/trunk/lib/Serialization/ASTReader.cpp

Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=330068=330067=330068=diff
==
--- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReader.cpp Fri Apr 13 14:12:33 2018
@@ -71,6 +71,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Lex/Token.h"
+#include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/ObjCMethodList.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/Sema.h"
@@ -97,11 +98,11 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/iterator_range.h"
+#include "llvm/Bitcode/BitCodes.h"
 #include "llvm/Bitcode/BitstreamReader.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compression.h"
@@ -133,8 +134,8 @@
 #include 
 
 using namespace clang;
-using namespace clang::serialization;
-using namespace clang::serialization::reader;
+using namespace serialization;
+using namespace reader;
 using llvm::BitstreamCursor;
 
 
//===--===//
@@ -463,7 +464,7 @@ static bool checkDiagnosticGroupMappings
   // errors because of options like -Werror.
   DiagnosticsEngine *MappingSources[] = { ,  };
 
-  for (DiagnosticsEngine *MappingSource : MappingSources) {
+  for (auto *MappingSource : MappingSources) {
 for (auto DiagIDMappingPair : MappingSource->getDiagnosticMappings()) {
   diag::kind DiagID = DiagIDMappingPair.first;
   Level CurLevel = Diags.getDiagnosticLevel(DiagID, SourceLocation());
@@ -581,9 +582,9 @@ static void
 collectMacroDefinitions(const PreprocessorOptions ,
 MacroDefinitionsMap ,
 SmallVectorImpl *MacroNames = nullptr) {
-  for (unsigned I = 0, N = PPOpts.Macros.size(); I != N; ++I) {
-StringRef Macro = PPOpts.Macros[I].first;
-bool IsUndef = PPOpts.Macros[I].second;
+  for (const auto  : PPOpts.Macros) {
+StringRef Macro = I.first;
+bool IsUndef = I.second;
 
 std::pair MacroPair = Macro.split('=');
 StringRef MacroName = MacroPair.first;
@@ -634,9 +635,8 @@ static bool checkPreprocessorOptions(con
   SmallVector ExistingMacroNames;
   collectMacroDefinitions(ExistingPPOpts, ExistingMacros, );
 
-  for (unsigned I = 0, N = ExistingMacroNames.size(); I != N; ++I) {
+  for (auto MacroName : ExistingMacroNames) {
 // Dig out the macro definition in the existing preprocessor options.
-StringRef MacroName = ExistingMacroNames[I];
 std::pair Existing = ExistingMacros[MacroName];
 
 // Check whether we know anything about this macro name or not.
@@ -702,8 +702,7 @@ static bool checkPreprocessorOptions(con
   }
 
   // Compute the #include and #include_macros lines we need.
-  for (unsigned I = 0, N = ExistingPPOpts.Includes.size(); I != N; ++I) {
-StringRef File = ExistingPPOpts.Includes[I];
+  for (const auto  : ExistingPPOpts.Includes) {
 if (File == ExistingPPOpts.ImplicitPCHInclude)
   continue;
 
@@ -716,8 +715,7 @@ static bool checkPreprocessorOptions(con
 SuggestedPredefines += "\"\n";
   }
 
-  for (unsigned I = 0, N = ExistingPPOpts.MacroIncludes.size(); I != N; ++I) {
-StringRef File = ExistingPPOpts.MacroIncludes[I];
+  for (const auto  : ExistingPPOpts.MacroIncludes) {
 if (std::find(PPOpts.MacroIncludes.begin(), PPOpts.MacroIncludes.end(),
   File)
 != PPOpts.MacroIncludes.end())
@@ -855,14 +853,14 @@ ASTSelectorLookupTrait::ReadData(Selecto
 
   // Load instance methods
   for (unsigned I = 0; I != NumInstanceMethods; ++I) {
-if (ObjCMethodDecl *Method = Reader.GetLocalDeclAs(
+if (auto *Method = Reader.GetLocalDeclAs(
 F, endian::readNext(d)))
   Result.Instance.push_back(Method);
   }
 
   // Load factory methods
   for (unsigned I = 0; I != NumFactoryMethods; ++I) {
-if (ObjCMethodDecl *Method = Reader.GetLocalDeclAs(
+if (auto *Method = Reader.GetLocalDeclAs(
 F, endian::readNext(d)))
   Result.Factory.push_back(Method);
   }
@@ -1087,7 +1085,7 @@ ASTDeclContextNameLookupTrait::internal_
 ASTDeclContextNameLookupTrait::ReadKey(const unsigned char *d, unsigned) {
   using namespace llvm::support;
 
-  auto Kind =