Re: [PATCH] D7639: Add readability-redundant-void-arg check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 37717.

http://reviews.llvm.org/D7639

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantVoidArgCheck.cpp
  clang-tidy/readability/RedundantVoidArgCheck.h
  test/clang-tidy/readability-redundant-void-arg.c
  test/clang-tidy/readability-redundant-void-arg.cpp

Index: test/clang-tidy/readability-redundant-void-arg.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-void-arg.cpp
@@ -0,0 +1,419 @@
+// RUN: %python %S/check_clang_tidy.py %s readability-redundant-void-arg %t
+
+#include 
+
+int foo();
+
+void bar();
+
+void bar2();
+
+extern "C" void ecfoo(void);
+
+extern "C" void ecfoo(void) {
+}
+
+extern int i;
+
+int j = 1;
+
+int foo(void) {
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant void argument list in function definition [readability-redundant-void-arg]
+// CHECK-FIXES: {{^}}int foo() {{{$}}
+return 0;
+}
+
+typedef unsigned int my_uint;
+
+typedef void my_void;
+
+// A function taking void and returning a pointer to function taking void
+// and returning int.
+int (*returns_fn_void_int(void))(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: {{.*}} in function declaration
+// CHECK-MESSAGES: :[[@LINE-2]]:34: warning: {{.*}} in function declaration
+// CHECK-FIXES: {{^}}int (*returns_fn_void_int())();{{$}}
+
+typedef int (*returns_fn_void_int_t(void))(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: {{.*}} in typedef
+// CHECK-MESSAGES: :[[@LINE-2]]:44: warning: {{.*}} in typedef
+// CHECK-FIXES: {{^}}typedef int (*returns_fn_void_int_t())();{{$}}
+
+int (*returns_fn_void_int(void))(void) {
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: {{.*}} in function definition
+// CHECK-MESSAGES: :[[@LINE-2]]:34: warning: {{.*}} in function definition
+// CHECK-FIXES: {{^}}int (*returns_fn_void_int())() {{{$}}
+  return nullptr;
+}
+
+// A function taking void and returning a pointer to a function taking void
+// and returning a pointer to a function taking void and returning void.
+void (*(*returns_fn_returns_fn_void_void(void))(void))(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:42: warning: {{.*}} in function declaration
+// CHECK-MESSAGES: :[[@LINE-2]]:49: warning: {{.*}} in function declaration
+// CHECK-MESSAGES: :[[@LINE-3]]:56: warning: {{.*}} in function declaration
+// CHECK-FIXES: {{^}}void (*(*returns_fn_returns_fn_void_void())())();{{$}}
+
+typedef void (*(*returns_fn_returns_fn_void_void_t(void))(void))(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:52: warning: {{.*}} in typedef
+// CHECK-MESSAGES: :[[@LINE-2]]:59: warning: {{.*}} in typedef
+// CHECK-MESSAGES: :[[@LINE-3]]:66: warning: {{.*}} in typedef
+// CHECK-FIXES: {{^}}typedef void (*(*returns_fn_returns_fn_void_void_t())())();{{$}}
+
+void (*(*returns_fn_returns_fn_void_void(void))(void))(void) {
+// CHECK-MESSAGES: :[[@LINE-1]]:42: warning: {{.*}} in function definition
+// CHECK-MESSAGES: :[[@LINE-2]]:49: warning: {{.*}} in function definition
+// CHECK-MESSAGES: :[[@LINE-3]]:56: warning: {{.*}} in function definition
+// CHECK-FIXES: {{^}}void (*(*returns_fn_returns_fn_void_void())())() {{{$}}
+return nullptr;
+}
+
+void bar(void) {
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: {{.*}} in function definition
+// CHECK-FIXES: {{^}}void bar() {{{$}}
+}
+
+void op_fn(int i) {
+}
+
+class gronk {
+public:
+  gronk();
+  ~gronk();
+
+void foo();
+void bar();
+void bar2
+();
+void operation(int i) { }
+
+private:
+int m_i;
+int *m_pi;
+float m_f;
+float *m_pf;
+double m_d;
+double *m_pd;
+
+void (*f1)(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*}} in field declaration
+// CHECK-FIXES: {{^}}void (*f1)();{{$}}
+
+  void (*op)(int i);
+
+  void (gronk::*p1)(void);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in field declaration
+  // CHECK-FIXES: {{^  }}void (gronk::*p1)();{{$}}
+
+  int (gronk::*p_mi);
+
+  void (gronk::*p2)(int);
+
+  void (*(*returns_fn_returns_fn_void_void(void))(void))(void);
+  // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: {{.*}} in function declaration
+  // CHECK-MESSAGES: :[[@LINE-2]]:51: warning: {{.*}} in function declaration
+  // CHECK-MESSAGES: :[[@LINE-3]]:58: warning: {{.*}} in function declaration
+  // CHECK-FIXES: {{^}}  void (*(*returns_fn_returns_fn_void_void())())();{{$}}
+
+  void (*(*(gronk::*returns_fn_returns_fn_void_void_mem)(void))(void))(void);
+  // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: {{.*}} in field declaration
+  // CHECK-MESSAGES: :[[@LINE-2]]:65: warning: {{.*}} in field declaration
+  // CHECK-MESSAGES: :[[@LINE-3]]:72: warning: {{.*}} in field declaration
+  // CHECK-FIXES: {{^}}  void (*(*(gronk::*returns_fn_returns_fn_void_void_mem)())())();{{$}}
+};
+
+int i;
+int *pi;
+void *pv = (void *) pi;
+float f;
+float *fi;
+double d;
+double *pd;
+
+void (*f1)(void);
+// CHECK-MESSAGES: :[[@LI

Re: [PATCH] D7639: Add readability-redundant-void-arg check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D7639#266332, @Eugene.Zelenko wrote:

> What is preventing to add this check to Clang-tidy? Just found another piece 
> of fresh C++ code in LLDB with (void) as argument list...


To be honest, I don't know.  This review had taken SO long to get 
approved.  At this point, the code is correct and I'd really like to get it 
committed and available for use instead of fussing with it any longer.


http://reviews.llvm.org/D7639



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

Re: [PATCH] D7639: Add readability-redundant-void-arg check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood marked 17 inline comments as done.
LegalizeAdulthood added a comment.

Can we get this committed?  I've addressed all comments.


http://reviews.llvm.org/D7639



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

Re: [PATCH] D10016: Refactor: Simplify boolean conditional return statements in lib/Frontend

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38309.
LegalizeAdulthood added a comment.

Update to latest


http://reviews.llvm.org/D10016

Files:
  lib/Frontend/VerifyDiagnosticConsumer.cpp

Index: lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -186,9 +186,7 @@
   Regex(RegexStr) { }
 
   bool isValid(std::string &Error) override {
-if (Regex.isValid(Error))
-  return true;
-return false;
+return Regex.isValid(Error);
   }
 
   bool match(StringRef S) override {


Index: lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -186,9 +186,7 @@
   Regex(RegexStr) { }
 
   bool isValid(std::string &Error) override {
-if (Regex.isValid(Error))
-  return true;
-return false;
+return Regex.isValid(Error);
   }
 
   bool match(StringRef S) override {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10016: Refactor: Simplify boolean conditional return statements in lib/Frontend

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

This changeset needs review; I'm unsure of who to add as explicit reviewers.


http://reviews.llvm.org/D10016



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

Re: [PATCH] D10020: Refactor: Simplify boolean conditional return statements in lib/Serialization

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38310.
LegalizeAdulthood added a comment.

Update to latest


http://reviews.llvm.org/D10020

Files:
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp

Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -3112,14 +3112,10 @@
   /// doesn't check whether the name has macros defined; use 
PublicMacroIterator
   /// to check that.
   bool isInterestingIdentifier(const IdentifierInfo *II, uint64_t MacroOffset) 
{
-if (MacroOffset ||
-II->isPoisoned() ||
-(IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) ||
-II->hasRevertedTokenIDToIdentifier() ||
-(NeedDecls && II->getFETokenInfo()))
-  return true;
-
-return false;
+return MacroOffset || II->isPoisoned() ||
+   (IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) ||
+   II->hasRevertedTokenIDToIdentifier() ||
+   (NeedDecls && II->getFETokenInfo());
   }
 
 public:
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -3956,10 +3956,7 @@
 
 case llvm::BitstreamEntry::SubBlock:
   if (Entry.ID == BlockID) {
-if (Cursor.EnterSubBlock(BlockID))
-  return true;
-// Found it!
-return false;
+return Cursor.EnterSubBlock(BlockID);
   }
   
   if (Cursor.SkipBlock())


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -3112,14 +3112,10 @@
   /// doesn't check whether the name has macros defined; use PublicMacroIterator
   /// to check that.
   bool isInterestingIdentifier(const IdentifierInfo *II, uint64_t MacroOffset) {
-if (MacroOffset ||
-II->isPoisoned() ||
-(IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) ||
-II->hasRevertedTokenIDToIdentifier() ||
-(NeedDecls && II->getFETokenInfo()))
-  return true;
-
-return false;
+return MacroOffset || II->isPoisoned() ||
+   (IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) ||
+   II->hasRevertedTokenIDToIdentifier() ||
+   (NeedDecls && II->getFETokenInfo());
   }
 
 public:
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -3956,10 +3956,7 @@
 
 case llvm::BitstreamEntry::SubBlock:
   if (Entry.ID == BlockID) {
-if (Cursor.EnterSubBlock(BlockID))
-  return true;
-// Found it!
-return false;
+return Cursor.EnterSubBlock(BlockID);
   }
   
   if (Cursor.SkipBlock())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10020: Refactor: Simplify boolean conditional return statements in lib/Serialization

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

This changeset needs review, but I'm unsure of who to add as explicit reviewers.


http://reviews.llvm.org/D10020



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

Re: [PATCH] D10021: Refactor: Simplify boolean conditional return statements in lib/StaticAnalyzer/Checkers

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38311.
LegalizeAdulthood added a comment.

Update to latest.
This changeset needs review; I am unsure who to add as specific reviewers.


http://reviews.llvm.org/D10021

Files:
  lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
  lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
  lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp

Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -235,12 +235,9 @@
 return false;
 
   // Run each of the checks on the conditions
-  if (containsMacro(cond) || containsEnum(cond)
-  || containsStaticLocal(cond) || containsBuiltinOffsetOf(cond)
-  || containsStmt(cond))
-return true;
-
-  return false;
+  return containsMacro(cond) || containsEnum(cond) ||
+ containsStaticLocal(cond) || containsBuiltinOffsetOf(cond) ||
+ containsStmt(cond);
 }
 
 // Returns true if the given CFGBlock is empty
Index: lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
@@ -404,10 +404,7 @@
 if (II == NSObjectII)
   break;
   }
-  if (!ID)
-return false;
-
-  return true;
+  return ID != nullptr;
 }
 
 /// \brief Returns true if the location is 'self'.
Index: lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
@@ -66,9 +66,8 @@
 // The type must be an array/pointer type.
 
 // This could be a null constant, which is allowed.
-if (E->isNullPointerConstant(ASTC, Expr::NPC_ValueDependentIsNull))
-  return true;
-return false;
+return static_cast(
+E->isNullPointerConstant(ASTC, Expr::NPC_ValueDependentIsNull));
   }
 
 public:
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1000,12 +1000,9 @@
   // Ex:  [NSData dataWithBytesNoCopy:bytes length:10];
   // (...unless a 'freeWhenDone' parameter is false, but that's checked later.)
   StringRef FirstSlot = Call.getSelector().getNameForSlot(0);
-  if (FirstSlot == "dataWithBytesNoCopy" ||
-  FirstSlot == "initWithBytesNoCopy" ||
-  FirstSlot == "initWithCharactersNoCopy")
-return true;
-
-  return false;
+  return FirstSlot == "dataWithBytesNoCopy" ||
+ FirstSlot == "initWithBytesNoCopy" ||
+ FirstSlot == "initWithCharactersNoCopy";
 }
 
 static Optional getFreeWhenDoneArg(const ObjCMethodCall &Call) {
Index: lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
@@ -201,12 +201,8 @@
 static bool isBadDeallocationArgument(const MemRegion *Arg) {
   if (!Arg)
 return false;
-  if (isa(Arg) ||
-  isa(Arg) ||
-  isa(Arg)) {
-return true;
-  }
-  return false;
+  return isa(Arg) || isa(Arg) ||
+ isa(Arg);
 }
 
 /// Given the address expression, retrieve the value it's pointing to. Assume
@@ -240,11 +236,7 @@
   DefinedOrUnknownSVal NoErr = Builder.evalEQ(State, NoErrVal,
  nonloc::SymbolVal(RetSym));
   ProgramStateRef ErrState = State->assume(NoErr, noError);
-  if (ErrState == State) {
-return true;
-  }
-
-  return false;
+  return ErrState == State;
 }
 
 // Report deallocator mismatch. Remove the region from tracking - reporting a
Index: lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -305,9 +305,7 @@
 const Stmt *Stmt2, bool IgnoreSideEffects) {
 
   if (!Stmt1 || !Stmt2) {
-if (!Stmt1 && !Stmt2)
-  return true;
-return false;
+return !Stmt1 && !Stmt2;
   }
 
   // If Stmt1 & Stmt2 are of different class then they are not
Index: lib/Stati

Re: [PATCH] D10023: Refactor: Simplify boolean conditional return statements in lib/StaticAnalyzer/Frontend

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38312.
LegalizeAdulthood added a comment.

Update to latest.
This changeset needs reveiw; I am unsure of who to add as specific reviewers.


http://reviews.llvm.org/D10023

Files:
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp

Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -83,10 +83,7 @@
 
   // For now, none of the static analyzer API is considered stable.
   // Versions must match exactly.
-  if (strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0)
-return true;
-
-  return false;
+  return strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
 void ClangCheckerRegistry::warnIncompatible(DiagnosticsEngine *diags,


Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -83,10 +83,7 @@
 
   // For now, none of the static analyzer API is considered stable.
   // Versions must match exactly.
-  if (strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0)
-return true;
-
-  return false;
+  return strcmp(versionString, CLANG_ANALYZER_API_VERSION_STRING) == 0;
 }
 
 void ClangCheckerRegistry::warnIncompatible(DiagnosticsEngine *diags,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10025: Refactor: Simplify boolean conditional return statements in clang-apply-replacements

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I do not have commit access; someone will need to commit this for me.


http://reviews.llvm.org/D10025



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

Re: [PATCH] D10025: Refactor: Simplify boolean conditional return statements in clang-apply-replacements

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38313.
LegalizeAdulthood added a comment.

Update to latest


http://reviews.llvm.org/D10025

Files:
  clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

Index: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -184,10 +184,7 @@
   }
 
   // Ask clang to deduplicate and report conflicts.
-  if (deduplicateAndDetectConflicts(GroupedReplacements, SM))
-return false;
-
-  return true;
+  return !deduplicateAndDetectConflicts(GroupedReplacements, SM);
 }
 
 bool applyReplacements(const FileToReplacementsMap &GroupedReplacements,


Index: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -184,10 +184,7 @@
   }
 
   // Ask clang to deduplicate and report conflicts.
-  if (deduplicateAndDetectConflicts(GroupedReplacements, SM))
-return false;
-
-  return true;
+  return !deduplicateAndDetectConflicts(GroupedReplacements, SM);
 }
 
 bool applyReplacements(const FileToReplacementsMap &GroupedReplacements,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10024: Refactor: Simplify boolean conditional return statements in tools/libclang

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38315.
LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.

Update to latest.
Update from comments.
I don't have commit access.


http://reviews.llvm.org/D10024

Files:
  tools/libclang/CIndex.cpp
  tools/libclang/Indexing.cpp
  tools/libclang/IndexingContext.cpp

Index: tools/libclang/IndexingContext.cpp
===
--- tools/libclang/IndexingContext.cpp
+++ tools/libclang/IndexingContext.cpp
@@ -804,10 +804,7 @@
   RefFileOccurrence RefOccur(FE, D);
   std::pair::iterator, bool>
   res = RefFileOccurrences.insert(RefOccur);
-  if (!res.second)
-return true; // already in map.
-
-  return false;
+  return !res.second; // already in map
 }
 
 const NamedDecl *IndexingContext::getEntityDecl(const NamedDecl *D) const {
Index: tools/libclang/Indexing.cpp
===
--- tools/libclang/Indexing.cpp
+++ tools/libclang/Indexing.cpp
@@ -669,9 +669,7 @@
 static bool topLevelDeclVisitor(void *context, const Decl *D) {
   IndexingContext &IdxCtx = *static_cast(context);
   IdxCtx.indexTopLevelDecl(D);
-  if (IdxCtx.shouldAbort())
-return false;
-  return true;
+  return !IdxCtx.shouldAbort();
 }
 
 static void indexTranslationUnit(ASTUnit &Unit, IndexingContext &IdxCtx) {
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -710,11 +710,8 @@
   return true;
 }
   }
-  
-  if (ShouldVisitBody && VisitCXXRecordDecl(D))
-return true;
-  
-  return false;
+
+  return ShouldVisitBody && VisitCXXRecordDecl(D);
 }
 
 bool CursorVisitor::VisitClassTemplatePartialSpecializationDecl(
@@ -939,11 +936,8 @@
   return true;
   }
 
-  if (ND->isThisDeclarationADefinition() &&
-  Visit(MakeCXCursor(ND->getBody(), StmtParent, TU, RegionOfInterest)))
-return true;
-
-  return false;
+  return ND->isThisDeclarationADefinition() &&
+ Visit(MakeCXCursor(ND->getBody(), StmtParent, TU, RegionOfInterest));
 }
 
 template 
@@ -6074,10 +6068,7 @@
 
   ++NextIdx;
   Lex.LexFromRawLexer(Tok);
-  if (Tok.is(tok::eof))
-return true;
-
-  return false;
+  return Tok.is(tok::eof);
 }
 
 static void annotatePreprocessorTokens(CXTranslationUnit TU,


Index: tools/libclang/IndexingContext.cpp
===
--- tools/libclang/IndexingContext.cpp
+++ tools/libclang/IndexingContext.cpp
@@ -804,10 +804,7 @@
   RefFileOccurrence RefOccur(FE, D);
   std::pair::iterator, bool>
   res = RefFileOccurrences.insert(RefOccur);
-  if (!res.second)
-return true; // already in map.
-
-  return false;
+  return !res.second; // already in map
 }
 
 const NamedDecl *IndexingContext::getEntityDecl(const NamedDecl *D) const {
Index: tools/libclang/Indexing.cpp
===
--- tools/libclang/Indexing.cpp
+++ tools/libclang/Indexing.cpp
@@ -669,9 +669,7 @@
 static bool topLevelDeclVisitor(void *context, const Decl *D) {
   IndexingContext &IdxCtx = *static_cast(context);
   IdxCtx.indexTopLevelDecl(D);
-  if (IdxCtx.shouldAbort())
-return false;
-  return true;
+  return !IdxCtx.shouldAbort();
 }
 
 static void indexTranslationUnit(ASTUnit &Unit, IndexingContext &IdxCtx) {
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -710,11 +710,8 @@
   return true;
 }
   }
-  
-  if (ShouldVisitBody && VisitCXXRecordDecl(D))
-return true;
-  
-  return false;
+
+  return ShouldVisitBody && VisitCXXRecordDecl(D);
 }
 
 bool CursorVisitor::VisitClassTemplatePartialSpecializationDecl(
@@ -939,11 +936,8 @@
   return true;
   }
 
-  if (ND->isThisDeclarationADefinition() &&
-  Visit(MakeCXCursor(ND->getBody(), StmtParent, TU, RegionOfInterest)))
-return true;
-
-  return false;
+  return ND->isThisDeclarationADefinition() &&
+ Visit(MakeCXCursor(ND->getBody(), StmtParent, TU, RegionOfInterest));
 }
 
 template 
@@ -6074,10 +6068,7 @@
 
   ++NextIdx;
   Lex.LexFromRawLexer(Tok);
-  if (Tok.is(tok::eof))
-return true;
-
-  return false;
+  return Tok.is(tok::eof);
 }
 
 static void annotatePreprocessorTokens(CXTranslationUnit TU,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10012: Refactor: Simplify boolean conditional return statements in lib/CodeGen

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38317.
LegalizeAdulthood added a comment.

Update to latest.
I do not have commit access.


http://reviews.llvm.org/D10012

Files:
  lib/CodeGen/BranchFolding.cpp
  lib/CodeGen/CodeGenPrepare.cpp
  lib/CodeGen/LiveDebugVariables.cpp
  lib/CodeGen/MachineRegisterInfo.cpp
  lib/CodeGen/PeepholeOptimizer.cpp
  lib/CodeGen/PseudoSourceValue.cpp
  lib/CodeGen/ScheduleDAGInstrs.cpp
  lib/CodeGen/StackProtector.cpp
  lib/CodeGen/TargetInstrInfo.cpp

Index: lib/CodeGen/TargetInstrInfo.cpp
===
--- lib/CodeGen/TargetInstrInfo.cpp
+++ lib/CodeGen/TargetInstrInfo.cpp
@@ -576,10 +576,7 @@
 MI2 = MRI.getUniqueVRegDef(Op2.getReg());
 
   // And they need to be in the trace (otherwise, they won't have a depth).
-  if (MI1 && MI2 && MI1->getParent() == MBB && MI2->getParent() == MBB)
-return true;
-
-  return false;
+  return MI1 && MI2 && MI1->getParent() == MBB && MI2->getParent() == MBB;
 }
 
 bool TargetInstrInfo::hasReassociableSibling(const MachineInstr &Inst,
@@ -600,25 +597,20 @@
   // 2. The previous instruction must have virtual register definitions for its
   //operands in the same basic block as Inst.
   // 3. The previous instruction's result must only be used by Inst.
-  if (MI1->getOpcode() == AssocOpcode && hasReassociableOperands(*MI1, MBB) &&
-  MRI.hasOneNonDBGUse(MI1->getOperand(0).getReg()))
-return true;
-
-  return false;
+  return MI1->getOpcode() == AssocOpcode &&
+ hasReassociableOperands(*MI1, MBB) &&
+ MRI.hasOneNonDBGUse(MI1->getOperand(0).getReg());
 }
 
 // 1. The operation must be associative and commutative.
 // 2. The instruction must have virtual register definitions for its
 //operands in the same basic block.
 // 3. The instruction must have a reassociable sibling.
 bool TargetInstrInfo::isReassociationCandidate(const MachineInstr &Inst,
bool &Commuted) const {
-  if (isAssociativeAndCommutative(Inst) &&
-  hasReassociableOperands(Inst, Inst.getParent()) &&
-  hasReassociableSibling(Inst, Commuted))
-return true;
-
-  return false;
+  return isAssociativeAndCommutative(Inst) &&
+ hasReassociableOperands(Inst, Inst.getParent()) &&
+ hasReassociableSibling(Inst, Commuted);
 }
 
 // The concept of the reassociation pass is that these operations can benefit
@@ -940,10 +932,7 @@
   // modification.
   const TargetLowering &TLI = *MF.getSubtarget().getTargetLowering();
   const TargetRegisterInfo *TRI = MF.getSubtarget().getRegisterInfo();
-  if (MI->modifiesRegister(TLI.getStackPointerRegisterToSaveRestore(), TRI))
-return true;
-
-  return false;
+  return MI->modifiesRegister(TLI.getStackPointerRegisterToSaveRestore(), TRI);
 }
 
 // Provide a global flag for disabling the PreRA hazard recognizer that targets
Index: lib/CodeGen/StackProtector.cpp
===
--- lib/CodeGen/StackProtector.cpp
+++ lib/CodeGen/StackProtector.cpp
@@ -465,10 +465,7 @@
 
   // Return if we didn't modify any basic blocks. i.e., there are no return
   // statements in the function.
-  if (!HasPrologue)
-return false;
-
-  return true;
+  return HasPrologue;
 }
 
 /// CreateFailBB - Create a basic block to jump to when the stack protector
Index: lib/CodeGen/ScheduleDAGInstrs.cpp
===
--- lib/CodeGen/ScheduleDAGInstrs.cpp
+++ lib/CodeGen/ScheduleDAGInstrs.cpp
@@ -462,11 +462,9 @@
 /// Return true if MI is an instruction we are unable to reason about
 /// (like a call or something with unmodeled side effects).
 static inline bool isGlobalMemoryObject(AliasAnalysis *AA, MachineInstr *MI) {
-  if (MI->isCall() || MI->hasUnmodeledSideEffects() ||
-  (MI->hasOrderedMemoryRef() &&
-   (!MI->mayLoad() || !MI->isInvariantLoad(AA
-return true;
-  return false;
+  return MI->isCall() || MI->hasUnmodeledSideEffects() ||
+ (MI->hasOrderedMemoryRef() &&
+  (!MI->mayLoad() || !MI->isInvariantLoad(AA)));
 }
 
 // This MI might have either incomplete info, or known to be unsafe
Index: lib/CodeGen/PseudoSourceValue.cpp
===
--- lib/CodeGen/PseudoSourceValue.cpp
+++ lib/CodeGen/PseudoSourceValue.cpp
@@ -50,9 +50,7 @@
 }
 
 bool PseudoSourceValue::mayAlias(const MachineFrameInfo *) const {
-  if (isGOT() || isConstantPool() || isJumpTable())
-return false;
-  return true;
+  return !(isGOT() || isConstantPool() || isJumpTable());
 }
 
 bool FixedStackPseudoSourceValue::isConstant(
Index: lib/CodeGen/PeepholeOptimizer.cpp
===
--- lib/CodeGen/PeepholeOptimizer.cpp
+++ lib/CodeGen/PeepholeOptimizer.cpp
@@ -686,10 +686,7 @@
   }
 
   // If we did not find a more suitable source, there is nothing to optimize.
-  if (CurSrcPair.Reg

Re: [PATCH] D10011: Refactor: Simplify boolean conditional return statements in lib/Basic

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38318.
LegalizeAdulthood added a comment.

Update to latest.
I do not have commit access.


http://reviews.llvm.org/D10011

Files:
  lib/Basic/Targets.cpp

Index: lib/Basic/Targets.cpp
===
--- lib/Basic/Targets.cpp
+++ lib/Basic/Targets.cpp
@@ -2704,22 +2704,22 @@
 
   // Enable popcnt if sse4.2 is enabled and popcnt is not explicitly disabled.
   auto I = Features.find("sse4.2");
-  if (I != Features.end() && I->getValue() == true &&
+  if (I != Features.end() && I->getValue() &&
   std::find(FeaturesVec.begin(), FeaturesVec.end(), "-popcnt") ==
   FeaturesVec.end())
 Features["popcnt"] = true;
 
   // Enable prfchw if 3DNow! is enabled and prfchw is not explicitly disabled.
   I = Features.find("3dnow");
-  if (I != Features.end() && I->getValue() == true &&
+  if (I != Features.end() && I->getValue() &&
   std::find(FeaturesVec.begin(), FeaturesVec.end(), "-prfchw") ==
   FeaturesVec.end())
 Features["prfchw"] = true;
 
   // Additionally, if SSE is enabled and mmx is not explicitly disabled,
   // then enable MMX.
   I = Features.find("sse");
-  if (I != Features.end() && I->getValue() == true &&
+  if (I != Features.end() && I->getValue() &&
   std::find(FeaturesVec.begin(), FeaturesVec.end(), "-mmx") ==
   FeaturesVec.end())
 Features["mmx"] = true;


Index: lib/Basic/Targets.cpp
===
--- lib/Basic/Targets.cpp
+++ lib/Basic/Targets.cpp
@@ -2704,22 +2704,22 @@
 
   // Enable popcnt if sse4.2 is enabled and popcnt is not explicitly disabled.
   auto I = Features.find("sse4.2");
-  if (I != Features.end() && I->getValue() == true &&
+  if (I != Features.end() && I->getValue() &&
   std::find(FeaturesVec.begin(), FeaturesVec.end(), "-popcnt") ==
   FeaturesVec.end())
 Features["popcnt"] = true;
 
   // Enable prfchw if 3DNow! is enabled and prfchw is not explicitly disabled.
   I = Features.find("3dnow");
-  if (I != Features.end() && I->getValue() == true &&
+  if (I != Features.end() && I->getValue() &&
   std::find(FeaturesVec.begin(), FeaturesVec.end(), "-prfchw") ==
   FeaturesVec.end())
 Features["prfchw"] = true;
 
   // Additionally, if SSE is enabled and mmx is not explicitly disabled,
   // then enable MMX.
   I = Features.find("sse");
-  if (I != Features.end() && I->getValue() == true &&
+  if (I != Features.end() && I->getValue() &&
   std::find(FeaturesVec.begin(), FeaturesVec.end(), "-mmx") ==
   FeaturesVec.end())
 Features["mmx"] = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10022: Refactor: Simplify boolean conditional return statements in lib/StaticAnalyzer/Core

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38319.
LegalizeAdulthood added a comment.

Update from latest


http://reviews.llvm.org/D10022

Files:
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  lib/StaticAnalyzer/Core/SValBuilder.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp

Index: lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -493,9 +493,7 @@
   if (LCtx != ELCtx) {
 // If the reaper's location context is a parent of the expression's
 // location context, then the expression value is now "out of scope".
-if (LCtx->isParentOf(ELCtx))
-  return false;
-return true;
+return !LCtx->isParentOf(ELCtx);
   }
 
   // If no statement is provided, everything is this and parent contexts is 
live.
Index: lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -394,10 +394,7 @@
   if (ToTy->isVoidType())
 return true;
 
-  if (ToTy != FromTy)
-return false;
-
-  return true;
+  return ToTy == FromTy;
 }
 
 // FIXME: should rewrite according to the cast kind.
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -690,14 +690,11 @@
 return true;
 
   CXXBasePaths Paths(false, false, false);
-  if (RD->lookupInBases(
-  [DeclName](const CXXBaseSpecifier *Specifier, CXXBasePath &Path) {
-return CXXRecordDecl::FindOrdinaryMember(Specifier, Path, 
DeclName);
-  },
-  Paths))
-return true;
-
-  return false;
+  return RD->lookupInBases(
+  [DeclName](const CXXBaseSpecifier *Specifier, CXXBasePath &Path) {
+return CXXRecordDecl::FindOrdinaryMember(Specifier, Path, DeclName);
+  },
+  Paths);
 }
 
 /// Returns true if the given C++ class is a container or iterator.


Index: lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -493,9 +493,7 @@
   if (LCtx != ELCtx) {
 // If the reaper's location context is a parent of the expression's
 // location context, then the expression value is now "out of scope".
-if (LCtx->isParentOf(ELCtx))
-  return false;
-return true;
+return !LCtx->isParentOf(ELCtx);
   }
 
   // If no statement is provided, everything is this and parent contexts is live.
Index: lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -394,10 +394,7 @@
   if (ToTy->isVoidType())
 return true;
 
-  if (ToTy != FromTy)
-return false;
-
-  return true;
+  return ToTy == FromTy;
 }
 
 // FIXME: should rewrite according to the cast kind.
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -690,14 +690,11 @@
 return true;
 
   CXXBasePaths Paths(false, false, false);
-  if (RD->lookupInBases(
-  [DeclName](const CXXBaseSpecifier *Specifier, CXXBasePath &Path) {
-return CXXRecordDecl::FindOrdinaryMember(Specifier, Path, DeclName);
-  },
-  Paths))
-return true;
-
-  return false;
+  return RD->lookupInBases(
+  [DeclName](const CXXBaseSpecifier *Specifier, CXXBasePath &Path) {
+return CXXRecordDecl::FindOrdinaryMember(Specifier, Path, DeclName);
+  },
+  Paths);
 }
 
 /// Returns true if the given C++ class is a container or iterator.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10022: Refactor: Simplify boolean conditional return statements in lib/StaticAnalyzer/Core

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:692-693
@@ -691,4 +691,2 @@
 
   CXXBasePaths Paths(false, false, false);
-  if (RD->lookupInBases(
-  [DeclName](const CXXBaseSpecifier *Specifier, CXXBasePath &Path) {

In this case it isn't a simple if-return, if-return, if-return.


http://reviews.llvm.org/D10022



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

Re: [PATCH] D10010: Refactor: Simplify boolean conditional return statements in lib/AST

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38321.
LegalizeAdulthood added a comment.

Update from latest.
I do not have commit access.


http://reviews.llvm.org/D10010

Files:
  lib/AST/ASTContext.cpp
  lib/AST/ASTDiagnostic.cpp
  lib/AST/Decl.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/Expr.cpp
  lib/AST/ExprConstant.cpp
  lib/AST/Type.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -1918,9 +1918,7 @@
 // CXXRecordDecl, use that one.
 RD = RD->getMostRecentDecl();
 // Nothing interesting to do if the inheritance attribute is already set.
-if (RD->hasAttr())
-  return false;
-return true;
+return !RD->hasAttr();
   }
   case ObjCObject:
 return cast(CanonicalType)->getBaseType()
@@ -2311,13 +2309,11 @@
   // Enumerated types are promotable to their compatible integer types
   // (C99 6.3.1.1) a.k.a. its underlying type (C++ [conv.prom]p2).
   if (const EnumType *ET = getAs()){
-if (this->isDependentType() || ET->getDecl()->getPromotionType().isNull()
-|| ET->getDecl()->isScoped())
-  return false;
-
-return true;
+return !(this->isDependentType() ||
+ ET->getDecl()->getPromotionType().isNull() ||
+ ET->getDecl()->isScoped());
   }
-  
+
   return false;
 }
 
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -6962,10 +6962,7 @@
   assert(E->getLHS()->getType()->isIntegralOrEnumerationType() &&
  E->getRHS()->getType()->isIntegralOrEnumerationType());
 
-  if (LHSResult.Failed && !Info.keepEvaluatingAfterFailure())
-return false; // Ignore RHS;
-
-  return true;
+  return !(LHSResult.Failed && !Info.keepEvaluatingAfterFailure());
 }
 
 bool DataRecursiveIntBinOpEvaluator::
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -3429,10 +3429,7 @@
   if (const ArraySubscriptExpr *ASE = dyn_cast(E))
 return ASE->getBase()->getType()->isVectorType();
 
-  if (isa(E))
-return true;
-
-  return false;
+  return isa(E);
 }
 
 bool Expr::refersToGlobalRegisterVar() const {
Index: lib/AST/DeclCXX.cpp
===
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -1861,10 +1861,7 @@
   // Is it the same as our our class type?
   CanQualType ClassTy 
 = Context.getCanonicalType(Context.getTagDeclType(getParent()));
-  if (ParamType.getUnqualifiedType() != ClassTy)
-return false;
-  
-  return true;  
+  return ParamType.getUnqualifiedType() == ClassTy;
 }
 
 const CXXConstructorDecl *CXXConstructorDecl::getInheritedConstructor() const {
Index: lib/AST/Decl.cpp
===
--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -2377,9 +2377,7 @@
 return false;
   }
 
-  if (isa(S) && cast(S)->body_empty())
-return true;
-  return false;
+  return isa(S) && cast(S)->body_empty();
 }
 
 bool FunctionDecl::isDefined(const FunctionDecl *&Definition) const {
@@ -2737,10 +2735,7 @@
   if (Redecl->isImplicit())
 return false;
 
-  if (!Redecl->isInlineSpecified() || Redecl->getStorageClass() == SC_Extern) 
-return true; // Not an inline definition
-
-  return false;
+  return !Redecl->isInlineSpecified() || Redecl->getStorageClass() == SC_Extern;
 }
 
 /// \brief For a function declaration in C or C++, determine whether this
Index: lib/AST/ASTDiagnostic.cpp
===
--- lib/AST/ASTDiagnostic.cpp
+++ lib/AST/ASTDiagnostic.cpp
@@ -1032,11 +1032,7 @@
   return false;
 }
 
-if (!Default->getType()->isReferenceType()) {
-  return true;
-}
-
-return false;
+return !Default->getType()->isReferenceType();
   }
 
   /// DiffNonTypes - Handles any template parameters not handled by DiffTypes
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -6654,15 +6654,12 @@
   // equivalent GCC vector types.
   const VectorType *First = FirstVec->getAs();
   const VectorType *Second = SecondVec->getAs();
-  if (First->getNumElements() == Second->getNumElements() &&
-  hasSameType(First->getElementType(), Second->getElementType()) &&
-  First->getVectorKind() != VectorType::AltiVecPixel &&
-  First->getVectorKind() != VectorType::AltiVecBool &&
-  Second->getVectorKind() != VectorType::AltiVecPixel &&
-  Second->getVectorKind() != VectorType::AltiVecBool)
-return true;
-
-  return false;
+  return First->getNumElements() == Second->getNumElements() &&
+ hasSameType(First->getElementType(), Second->getElementType()) &&
+ First->getVectorKind() != VectorType::AltiVecPixel &&
+ First->getVectorKind() != VectorTyp

Re: [PATCH] D10017: Refactor: Simplify boolean conditional return statements in lib/Lex

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38322.
LegalizeAdulthood added a comment.

Update to latest.
I do not have commit access.


http://reviews.llvm.org/D10017

Files:
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPMacroExpansion.cpp

Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -597,9 +597,7 @@
   Brackets.pop_back();
 }
   }
-  if (!Brackets.empty())
-return false;
-  return true;
+  return Brackets.empty();
 }
 
 /// GenerateNewArgTokens - Returns true if OldTokens can be converted to a new
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -2045,13 +2045,9 @@
   }
 
   // #define inline
-  if (MacroName.isOneOf(tok::kw_extern, tok::kw_inline, tok::kw_static,
-tok::kw_const) &&
-  MI->getNumTokens() == 0) {
-return true;
-  }
-
-  return false;
+  return MacroName.isOneOf(tok::kw_extern, tok::kw_inline, tok::kw_static,
+   tok::kw_const) &&
+ MI->getNumTokens() == 0;
 }
 
 /// HandleDefineDirective - Implements \#define.  This consumes the entire 
macro


Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -597,9 +597,7 @@
   Brackets.pop_back();
 }
   }
-  if (!Brackets.empty())
-return false;
-  return true;
+  return Brackets.empty();
 }
 
 /// GenerateNewArgTokens - Returns true if OldTokens can be converted to a new
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -2045,13 +2045,9 @@
   }
 
   // #define inline
-  if (MacroName.isOneOf(tok::kw_extern, tok::kw_inline, tok::kw_static,
-tok::kw_const) &&
-  MI->getNumTokens() == 0) {
-return true;
-  }
-
-  return false;
+  return MacroName.isOneOf(tok::kw_extern, tok::kw_inline, tok::kw_static,
+   tok::kw_const) &&
+ MI->getNumTokens() == 0;
 }
 
 /// HandleDefineDirective - Implements \#define.  This consumes the entire macro
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10014: Refactor: Simplify boolean conditional return statements in lib/Edit

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38323.
LegalizeAdulthood added a comment.

Update from latest.
I do not have commit access.


http://reviews.llvm.org/D10014

Files:
  lib/Edit/RewriteObjCFoundationAPI.cpp

Index: lib/Edit/RewriteObjCFoundationAPI.cpp
===
--- lib/Edit/RewriteObjCFoundationAPI.cpp
+++ lib/Edit/RewriteObjCFoundationAPI.cpp
@@ -888,53 +888,29 @@
 // make it broadly available.
 static bool subscriptOperatorNeedsParens(const Expr *FullExpr) {
   const Expr* Expr = FullExpr->IgnoreImpCasts();
-  if (isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(FullExpr) ||
-  isa(Expr) ||
-  isa(Expr))
-return false;
-
-  return true;
+  return !(isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(FullExpr) ||
+   isa(Expr) || isa(Expr));
 }
 static bool castOperatorNeedsParens(const Expr *FullExpr) {
   const Expr* Expr = FullExpr->IgnoreImpCasts();
-  if (isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(FullExpr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr))
-return false;
-
-  return true;
+  return !(isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(FullExpr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr));
 }
 
 static void objectifyExpr(const Expr *E, Commit &commit) {


Index: lib/Edit/RewriteObjCFoundationAPI.cpp
===
--- lib/Edit/RewriteObjCFoundationAPI.cpp
+++ lib/Edit/RewriteObjCFoundationAPI.cpp
@@ -888,53 +888,29 @@
 // make it broadly available.
 static bool subscriptOperatorNeedsParens(const Expr *FullExpr) {
   const Expr* Expr = FullExpr->IgnoreImpCasts();
-  if (isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(FullExpr) ||
-  isa(Expr) ||
-  isa(Expr))
-return false;
-
-  return true;
+  return !(isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(FullExpr) ||
+   isa(Expr) || isa(Expr));
 }
 static bool castOperatorNeedsParens(const Expr *FullExpr) {
   const Expr* Expr = FullExpr->IgnoreImpCasts();
-  if (isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(FullExpr) ||
-  isa(Expr) ||
-  isa(Expr) ||
-  isa(Expr))
-return false;
-
-  return true;
+  return !(isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr) || isa(FullExpr) ||
+   isa(Expr) || isa(Expr) ||
+   isa(Expr));
 }
 
 static void objectifyExpr(const Expr *E, Commit &commit) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10014: Refactor: Simplify boolean conditional return statements in lib/Edit

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D10014#178294, @calabrese wrote:

> I like the idea of all of these changes (this change list and the others), 
> but just a suggestion -- it looks like when the branch of the original 
> condition returns false, as was the case in the original code here, the 
> internal expressions of the check are transformed via De Morgan's laws in the 
> new code. IMO, this transformation sometimes makes things more complicated 
> rather than less, where by "more complicated" I mean that the transformation 
> produces an overall expression that is constructed from a considerably 
> greater number of logical operations. For instance, here it changed a bunch 
> of || expressions into a bunch of && expressions, with each operand 
> individually prefixed with "!". The equivalent without having applied De 
> Morgan's laws would just have left all of the || expressions as-is and added 
> a single ! around the overall expression parenthesized. It would have also 
> looked much more similar to the original code.
>
> My general thought here is that it might make sense when performing this kind 
> of transformation to first see if the translation via De Morgan's laws 
> actually produces an overall expression that has fewer logical 
> subexpressions, otherwise prefer the minimal transformation of a ! around a 
> parenthesized expression.


This should be addressed in the latest diff.


http://reviews.llvm.org/D10014



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

Re: [PATCH] D10019: Refactor: Simplify boolean conditional return statements in lib/Sema

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 38324.
LegalizeAdulthood added a comment.

Update from latest.
Update from comments.
I do not have commit access.


http://reviews.llvm.org/D10019

Files:
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaDeclObjC.cpp
  lib/Sema/SemaExceptionSpec.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaFixItUtils.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaOverload.cpp

Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -214,15 +214,12 @@
   // array-to-pointer or function-to-pointer implicit conversions, so
   // check for their presence as well as checking whether FromType is
   // a pointer.
-  if (getToType(1)->isBooleanType() &&
-  (getFromType()->isPointerType() ||
-   getFromType()->isObjCObjectPointerType() ||
-   getFromType()->isBlockPointerType() ||
-   getFromType()->isNullPtrType() ||
-   First == ICK_Array_To_Pointer || First == ICK_Function_To_Pointer))
-return true;
-
-  return false;
+  return getToType(1)->isBooleanType() &&
+ (getFromType()->isPointerType() ||
+  getFromType()->isObjCObjectPointerType() ||
+  getFromType()->isBlockPointerType() ||
+  getFromType()->isNullPtrType() || First == ICK_Array_To_Pointer ||
+  First == ICK_Function_To_Pointer);
 }
 
 /// isPointerConversionToVoidPointer - Determines whether this
@@ -1884,11 +1881,7 @@
 
   // An rvalue of type bool can be converted to an rvalue of type int,
   // with false becoming zero and true becoming one (C++ 4.5p4).
-  if (FromType->isBooleanType() && To->getKind() == BuiltinType::Int) {
-return true;
-  }
-
-  return false;
+  return FromType->isBooleanType() && To->getKind() == BuiltinType::Int;
 }
 
 /// IsFloatingPointPromotion - Determines whether the conversion from
@@ -2806,11 +2799,8 @@
 static bool isNonTrivialObjCLifetimeConversion(Qualifiers FromQuals,
Qualifiers ToQuals) {
   // Converting anything to const __unsafe_unretained is trivial.
-  if (ToQuals.hasConst() && 
-  ToQuals.getObjCLifetime() == Qualifiers::OCL_ExplicitNone)
-return false;
-
-  return true;
+  return !(ToQuals.hasConst() &&
+   ToQuals.getObjCLifetime() == Qualifiers::OCL_ExplicitNone);
 }
 
 /// IsQualificationConversion - Determines whether the conversion from
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -6288,9 +6288,7 @@
 return true;
   DSAStackTy::DSAVarData DVarPrivate =
   Stack->hasDSA(VD, isOpenMPPrivate, MatchesAlways(), false);
-  if (DVarPrivate.CKind != OMPC_unknown)
-return true;
-  return false;
+  return DVarPrivate.CKind != OMPC_unknown;
 }
 return false;
   }
Index: lib/Sema/SemaFixItUtils.cpp
===
--- lib/Sema/SemaFixItUtils.cpp
+++ lib/Sema/SemaFixItUtils.cpp
@@ -42,10 +42,8 @@
   const CanQualType FromUnq = From.getUnqualifiedType();
   const CanQualType ToUnq = To.getUnqualifiedType();
 
-  if ((FromUnq == ToUnq || (S.IsDerivedFrom(FromUnq, ToUnq)) ) &&
-  To.isAtLeastAsQualifiedAs(From))
-return true;
-  return false;
+  return (FromUnq == ToUnq || (S.IsDerivedFrom(FromUnq, ToUnq))) &&
+ To.isAtLeastAsQualifiedAs(From);
 }
 
 bool ConversionFixItGenerator::tryToFixConversion(const Expr *FullExpr,
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -2314,11 +2314,9 @@
   return true;
 }
 
-if (CheckAllocationAccess(StartLoc, SourceRange(), Found.getNamingClass(),
-  Matches[0], Diagnose) == AR_inaccessible)
-  return true;
-
-return false;
+return CheckAllocationAccess(StartLoc, SourceRange(),
+ Found.getNamingClass(), Matches[0],
+ Diagnose) == AR_inaccessible;
 
   // We found multiple suitable operators;  complain about the ambiguity.
   } else if (!Matches.empty()) {
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7716,9 +7716,7 @@
 return !S.getLangOpts().CPlusPlus;
   }
 
-  if (checkArithmeticIncompletePointerType(S, Loc, Operand)) return false;
-
-  return true;
+  return !checkArithmeticIncompletePointerType(S, Loc, Operand);
 }
 
 /// \brief Check the validity of a binary arithmetic operation w.r.t. pointer
@@ -8424,10 +8422,7 @@
 return false;
 
   QualType R = Method->getReturnType();
-  if (!R->isScalarType())
-return false;
-
-  r

Re: [PATCH] D7639: Add readability-redundant-void-arg check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D7639#275504, @sbenza wrote:

> Just fyi, I am looking at this diff. It is very large with a lot of rounds of 
> comments and I didn't have the context.
>  I don't know if I should giving comments at this point of the change, but 
> here it is.
>  Have you considered matching on typeLoc() instead of having a large list of 
> different cases?
>  For example, if you match `typeLoc(loc(functionType()))` it will match all 
> the places where a function type is mentioned, including things like 
> `static_cast`, variable declarations, lambda return type declarations, 
> etc. Might help remove redundancy in the check.


That occurred to me and I did an experiment and it didn't work out.  I forget 
the exact details now as it was months ago and this review has been sitting 
here languishing with a correct implementation as-is.  I really just want to 
get this committed and make incremental improvement, instead of re-evaluating 
the entire thing from scratch at this time.


http://reviews.llvm.org/D7639



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

Re: [PATCH] D7639: Add readability-redundant-void-arg check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I mean, look at this review.  I created it on Feb 13th 2015.  It's been getting 
ground through the review process for 8 months.  Why can't we move forward?


http://reviews.llvm.org/D7639



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:88
@@ +87,3 @@
+}
+
+bool containsDelimiter(StringRef Bytes, const std::string &Delimiter) {

alexfh wrote:
> aaron.ballman wrote:
> > I think Alex's point is: why not R"('\"?x01)" (removing the need for lit)?
> Exactly, I was only talking about `lit`, not about using the raw string 
> literal.
It was looking a little busy to my eyes with the raw string " and the quoted " 
close together.  It isn't necessary, but IMO improves readability.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:96
@@ +95,3 @@
+  std::string Delimiter;
+  for (int Counter = 0; containsDelimiter(Bytes, Delimiter); ++Counter) {
+Delimiter = (Counter == 0) ? "lit" : "lit" + std::to_string(Counter);

alexfh wrote:
> Please address my comment above that relates to this code. Specifically, I 
> find this generic algorithm unnecessarily inefficient and too rigid, i.e. one 
> can't configure a custom set of delimiters that don't follow the 
>  pattern. I suggest using a list of pre-concatenated pairs of 
> strings (like `R"lit(` and `)lit"`).
Raw strings are new and not many people are using them because the don't have a 
good way to automatically convert disgusting quoted strings to raw strings.  So 
I don't think it is reasonable to draw conclusions by looking in existing code 
bases for raw strings.

We're having the same conversation we've had before.  I'm trying to do a basic 
check and get things working properly and the review comments are tending 
towards a desire for some kind of perfection.

I don't see why we have to make the perfect the enemy of the good.  Nothing you 
are suggesting **must** be present now in order for this check to function 
properly and reasonably.


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:21-22
@@ -20,71 +20,4 @@
 /// them to use the appropriate boolean expression directly.
 ///
-/// Examples:
-///
-/// ===  
-/// Initial expression   Result
-/// ---  
-/// `if (b == true)` `if (b)`
-/// `if (b == false)``if (!b)`
-/// `if (b && true)` `if (b)`
-/// `if (b && false)``if (false)`
-/// `if (b || true)` `if (true)`
-/// `if (b || false)``if (b)`
-/// `e ? true : false`   `e`
-/// `e ? false : true`   `!e`
-/// `if (true) t(); else f();`   `t();`
-/// `if (false) t(); else f();`  `f();`
-/// `if (e) return true; else return false;` `return e;`
-/// `if (e) return false; else return true;` `return !e;`
-/// `if (e) b = true; else b = false;`   `b = e;`
-/// `if (e) b = false; else b = true;`   `b = !e;`
-/// `if (e) return true; return false;`  `return e;`
-/// `if (e) return false; return true;`  `return !e;`
-/// ===  
-///
-/// The resulting expression `e` is modified as follows:
-///   1. Unnecessary parentheses around the expression are removed.
-///   2. Negated applications of `!` are eliminated.
-///   3. Negated applications of comparison operators are changed to use the
-///  opposite condition.
-///   4. Implicit conversions of pointer to `bool` are replaced with explicit
-///  comparisons to `nullptr`.
-///   5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
-///   6. Object expressions with `explicit operator bool` conversion operators
-///  are replaced with explicit casts to `bool`.
-///
-/// Examples:
-///   1. The ternary assignment `bool b = (i < 0) ? true : false;` has 
redundant
-///  parentheses and becomes `bool b = i < 0;`.
-///
-///   2. The conditional return `if (!b) return false; return true;` has an
-///  implied double negation and becomes `return b;`.
-///
-///   3. The conditional return `if (i < 0) return false; return true;` becomes
-///  `return i >= 0;`.
-///
-///  The conditional return `if (i != 0) return false; return true;` 
becomes
-///  `return i == 0;`.
-///
-///   4. The conditional return `if (p) return true; return false;` has an
-///  implicit conversion of a pointer to `bool` and becomes
-///  `return p != nullptr;`.
-///
-///  The ternary assignment `bool b = (i & 1) ? true : false;` has an
-///  implicit conversion of `i & 1` to `bool` and becomes
-///  `bool b = static_cast(i & 1);`.
-///
-///   5. The conditional return `if (i & 1) return true; else return false;` 
has
-///  an implicit conversion of an integer quantity `i & 1` to `bool` and
-///  becomes `return static_cast(i & 1);`
-///
-///   6. Given `struct X { explicit operator bool(); };`, and an instance `x` 
of
-///  `struct X`, the conditional return `if (x) return true; return false;`
-///  becomes `return static_cast(x);`
-///
-/// When a conditional boolean return or assignment appears at the end of a
-/// chain of `if`, `else if` statements, the conditional statement is left
-/// unchanged unless the option `ChainedConditionalReturn` or
-/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
-/// The default value for both options is zero.
-///
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html

aaron.ballman wrote:
> I think I can agree with that, but I also think it depends a lot on what the 
> purpose to the check is. If it's "improve readability regarding parens" + 
> options that control when to remove or add, that makes sense to me. 
> Otherwise, I think segregating the checks into one that removes (plus 
> options) and one that adds (plus options) makes sense -- even if they perhaps 
> use the same underlying heuristic engine and are simply surfaced as separate 
> checks.
This check isn't at all about the readability of parens, so it doesn't make 
sense for this check to be concerned with it IMO.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16526: Add hasRetValue narrowing matcher for returnStmt

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D16526#334938, @aaron.ballman wrote:

> I'm of two minds on this [...]


I just need a thumbs up/down on this.

Thumbs down, I discard the review.

Thumbs up, you take the patch.

Patch by Richard Thomson.


http://reviews.llvm.org/D16526



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:21-22
@@ -20,71 +20,4 @@
 /// them to use the appropriate boolean expression directly.
 ///
-/// Examples:
-///
-/// ===  
-/// Initial expression   Result
-/// ---  
-/// `if (b == true)` `if (b)`
-/// `if (b == false)``if (!b)`
-/// `if (b && true)` `if (b)`
-/// `if (b && false)``if (false)`
-/// `if (b || true)` `if (true)`
-/// `if (b || false)``if (b)`
-/// `e ? true : false`   `e`
-/// `e ? false : true`   `!e`
-/// `if (true) t(); else f();`   `t();`
-/// `if (false) t(); else f();`  `f();`
-/// `if (e) return true; else return false;` `return e;`
-/// `if (e) return false; else return true;` `return !e;`
-/// `if (e) b = true; else b = false;`   `b = e;`
-/// `if (e) b = false; else b = true;`   `b = !e;`
-/// `if (e) return true; return false;`  `return e;`
-/// `if (e) return false; return true;`  `return !e;`
-/// ===  
-///
-/// The resulting expression `e` is modified as follows:
-///   1. Unnecessary parentheses around the expression are removed.
-///   2. Negated applications of `!` are eliminated.
-///   3. Negated applications of comparison operators are changed to use the
-///  opposite condition.
-///   4. Implicit conversions of pointer to `bool` are replaced with explicit
-///  comparisons to `nullptr`.
-///   5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
-///   6. Object expressions with `explicit operator bool` conversion operators
-///  are replaced with explicit casts to `bool`.
-///
-/// Examples:
-///   1. The ternary assignment `bool b = (i < 0) ? true : false;` has 
redundant
-///  parentheses and becomes `bool b = i < 0;`.
-///
-///   2. The conditional return `if (!b) return false; return true;` has an
-///  implied double negation and becomes `return b;`.
-///
-///   3. The conditional return `if (i < 0) return false; return true;` becomes
-///  `return i >= 0;`.
-///
-///  The conditional return `if (i != 0) return false; return true;` 
becomes
-///  `return i == 0;`.
-///
-///   4. The conditional return `if (p) return true; return false;` has an
-///  implicit conversion of a pointer to `bool` and becomes
-///  `return p != nullptr;`.
-///
-///  The ternary assignment `bool b = (i & 1) ? true : false;` has an
-///  implicit conversion of `i & 1` to `bool` and becomes
-///  `bool b = static_cast(i & 1);`.
-///
-///   5. The conditional return `if (i & 1) return true; else return false;` 
has
-///  an implicit conversion of an integer quantity `i & 1` to `bool` and
-///  becomes `return static_cast(i & 1);`
-///
-///   6. Given `struct X { explicit operator bool(); };`, and an instance `x` 
of
-///  `struct X`, the conditional return `if (x) return true; return false;`
-///  becomes `return static_cast(x);`
-///
-/// When a conditional boolean return or assignment appears at the end of a
-/// chain of `if`, `else if` statements, the conditional statement is left
-/// unchanged unless the option `ChainedConditionalReturn` or
-/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
-/// The default value for both options is zero.
-///
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > I think I can agree with that, but I also think it depends a lot on what 
> > > the purpose to the check is. If it's "improve readability regarding 
> > > parens" + options that control when to remove or add, that makes sense to 
> > > me. Otherwise, I think segregating the checks into one that removes (plus 
> > > options) and one that adds (plus options) makes sense -- even if they 
> > > perhaps use the same underlying heuristic engine and are simply surfaced 
> > > as separate checks.
> > This check isn't at all about the readability of parens, so it doesn't make 
> > sense for this check to be concerned with it IMO.
> Agreed; trying to suss out what the appropriate design for this particular 
> check is. I think I've talked myself into "don't touch parens here."
Currently in this patch, parens are added when the expression compared to 
`nullptr` or `0` is a binary operator.

I think that is the right thing to do here so we get:

```
bool b = (i & 1) == 0;
```

instead of

```
bool b = i & 1 == 0;
```



http://r

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:21-22
@@ -20,71 +20,4 @@
 /// them to use the appropriate boolean expression directly.
 ///
-/// Examples:
-///
-/// ===  
-/// Initial expression   Result
-/// ---  
-/// `if (b == true)` `if (b)`
-/// `if (b == false)``if (!b)`
-/// `if (b && true)` `if (b)`
-/// `if (b && false)``if (false)`
-/// `if (b || true)` `if (true)`
-/// `if (b || false)``if (b)`
-/// `e ? true : false`   `e`
-/// `e ? false : true`   `!e`
-/// `if (true) t(); else f();`   `t();`
-/// `if (false) t(); else f();`  `f();`
-/// `if (e) return true; else return false;` `return e;`
-/// `if (e) return false; else return true;` `return !e;`
-/// `if (e) b = true; else b = false;`   `b = e;`
-/// `if (e) b = false; else b = true;`   `b = !e;`
-/// `if (e) return true; return false;`  `return e;`
-/// `if (e) return false; return true;`  `return !e;`
-/// ===  
-///
-/// The resulting expression `e` is modified as follows:
-///   1. Unnecessary parentheses around the expression are removed.
-///   2. Negated applications of `!` are eliminated.
-///   3. Negated applications of comparison operators are changed to use the
-///  opposite condition.
-///   4. Implicit conversions of pointer to `bool` are replaced with explicit
-///  comparisons to `nullptr`.
-///   5. Implicit casts to `bool` are replaced with explicit casts to `bool`.
-///   6. Object expressions with `explicit operator bool` conversion operators
-///  are replaced with explicit casts to `bool`.
-///
-/// Examples:
-///   1. The ternary assignment `bool b = (i < 0) ? true : false;` has 
redundant
-///  parentheses and becomes `bool b = i < 0;`.
-///
-///   2. The conditional return `if (!b) return false; return true;` has an
-///  implied double negation and becomes `return b;`.
-///
-///   3. The conditional return `if (i < 0) return false; return true;` becomes
-///  `return i >= 0;`.
-///
-///  The conditional return `if (i != 0) return false; return true;` 
becomes
-///  `return i == 0;`.
-///
-///   4. The conditional return `if (p) return true; return false;` has an
-///  implicit conversion of a pointer to `bool` and becomes
-///  `return p != nullptr;`.
-///
-///  The ternary assignment `bool b = (i & 1) ? true : false;` has an
-///  implicit conversion of `i & 1` to `bool` and becomes
-///  `bool b = static_cast(i & 1);`.
-///
-///   5. The conditional return `if (i & 1) return true; else return false;` 
has
-///  an implicit conversion of an integer quantity `i & 1` to `bool` and
-///  becomes `return static_cast(i & 1);`
-///
-///   6. Given `struct X { explicit operator bool(); };`, and an instance `x` 
of
-///  `struct X`, the conditional return `if (x) return true; return false;`
-///  becomes `return static_cast(x);`
-///
-/// When a conditional boolean return or assignment appears at the end of a
-/// chain of `if`, `else if` statements, the conditional statement is left
-/// unchanged unless the option `ChainedConditionalReturn` or
-/// `ChainedConditionalAssignment`, respectively, is specified as non-zero.
-/// The default value for both options is zero.
-///
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html

aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > I think I can agree with that, but I also think it depends a lot on 
> > > > > what the purpose to the check is. If it's "improve readability 
> > > > > regarding parens" + options that control when to remove or add, that 
> > > > > makes sense to me. Otherwise, I think segregating the checks into one 
> > > > > that removes (plus options) and one that adds (plus options) makes 
> > > > > sense -- even if they perhaps use the same underlying heuristic 
> > > > > engine and are simply surfaced as separate checks.
> > > > This check isn't at all about the readability of parens, so it doesn't 
> > > > make sense for this check to be concerned with it IMO.
> > > Agreed; trying to suss out what the appropriate design for this 
> > > particular check is. I think I've talked myself into "don't touch parens 
> > > here."
> > Currently in this patch, parens are added when the expression compared to 
> > `nullptr` or `0` is a binary operator.
> > 
> > I think that

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:366
@@ +365,3 @@
+  binaryOperator(
+  isExpansionInMainFile(), hasOperatorName(OperatorName),
+  hasLHS(allOf(

aaron.ballman wrote:
> Sorry for not noticing this earlier, but since we have two other in-flight 
> patches I reviewed this morning, it caught my attention: we should not be 
> using isExpansionInMainFile, but instead testing the source location of the 
> matches to see if they're in a macro expansion. This is usually done with 
> something like `Result.SourceManager->isMacroBodyExpansion(SomeLoc)`.
> 
> I realize this is existing code and not your problem, but it should be fixed 
> in a follow-on patch (by someone) and include some macro test cases.
There is an open bug on this that I will address.  And while this is existing 
code, I am the author of the original check :).

I also experienced a problem when running this check on some code that did 
something like:

```
#define FOO (par[1] > 4)

// ...
bool f() {
  if (!FOO) {
return true;
  } else {
return false;
  }
}
```

So it needs improvement w.r.t. macros and that is also on my to-do list.

I'm trying to do things in incremental steps and not giant changes.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I do not have commit access.  If someone could commit this for me, I would 
appreciate it.

Patch by Richard Thomson.

Thanks.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16786: [Clang-tidy] Make modernize-redundant-void-arg working with included files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I am already working on fixing this bug (isn't PR25894 
 already assigned to me?), but 
what's missing from this patch is an automated test that proves it works.

In order to write such an automated test, I believe some enhancements are 
needed to the `check_clang_tidy.py` script before such a test can be written.


Repository:
  rL LLVM

http://reviews.llvm.org/D16786



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

Re: [PATCH] D16700: [Clang-tidy] Make null pointer literals for fixes configurable for two checks

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/ImplicitBoolCastCheck.h:32
@@ -30,1 +31,3 @@
+Options.get("NullPointerLiteral",
+Context->getLangOpts().CPlusPlus11 ? "nullptr" : "0")) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;

aaron.ballman wrote:
> I know you are following the pattern used in the code here, but I think this 
> class needs a storeOptions() function as well. See AssertSideEffectCheck as 
> an example.
This will need rebasing on the existing code, which is using "NULL" as the 
pre-C++11 fallback default, not "0".


Comment at: clang-tidy/readability/ImplicitBoolCastCheck.h:36-38
@@ -32,1 +35,5 @@
 
+  StringRef getNullPointerLiteral() const {
+return NullPointerLiteral;
+  }
+

I don't understand why the checks need a public getter for the nullptr literal 
being used.


Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:175
@@ -174,2 +174,3 @@
 
-std::string replacementExpression(const MatchFinder::MatchResult &Result,
+std::string replacementExpression(SimplifyBooleanExprCheck *Check,
+  const MatchFinder::MatchResult &Result,

aaron.ballman wrote:
> Check can be a const pointer.
Passing the check in here is overkill.  This helper function isn't going to 
ever be used for multiple checks and the only thing you ever do with the check 
is get the nullptr literal, so just pass in the thing it needs directly.

This will also need to be rebased onto the current code.


Comment at: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst:79-81
@@ -78,1 +78,4 @@
 
+Null pointer literal for fixes could be changed with option
+``NullPointerLiteral``. The default value for C++11 or later is ``nullptr``, 
for
+ C++98/C++03 - ``0``.

The wording here is awkward.  Instead, I suggest:

```
The option ``NullPointerLiteral`` configures the text used for comparisons of 
pointers
against zero to synthesize boolean expressions.  The default value for C++11 or 
later
is ``nullptr``, otherwise the value is ``NULL``.
```

It's subjective, but my experience is that pre C++11 code bases prefer `NULL` 
over `0`.


Repository:
  rL LLVM

http://reviews.llvm.org/D16700



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

Re: [PATCH] D16794: [Clang-tidy] Make readability-simplify-boolean-expr working with included files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Again, isn't this already assigned to me in the bug tracker?

In general, my assumption in bug trackers is that if someone has assigned the 
bug to themselves, then they are working on it.


Repository:
  rL LLVM

http://reviews.llvm.org/D16794



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

Re: [PATCH] D16794: [Clang-tidy] Make readability-simplify-boolean-expr working with included files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D16794#341552, @aaron.ballman wrote:

> Missing a test case for this functionality. The isExpansionInMainFile() was 
> used to silence the diagnostic in macros, but it was pointed out during 
> review that this should be fixed and it seems to have fallen through the 
> cracks. Can you also add tests for macros to ensure the behavior is 
> acceptable there still?
>
> See http://reviews.llvm.org/D8996 for more context.


It hasn't fallen through the cracks, I've been having an offline discussion 
with Alexander Kornienko about how to get automated checks to work for fixits 
applied to headers.


Repository:
  rL LLVM

http://reviews.llvm.org/D16794



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

Re: [PATCH] D16259: Add clang-tidy readability-redundant-control-flow check

[Richard via cfe-commits]

In article ,
Aaron Ballman  writes:

> On Wed, Feb 3, 2016 at 3:57 PM, Kim Gräsman  wrote:
> > On Mon, Feb 1, 2016 at 4:32 PM, Aaron Ballman 
> wrote:
> >>
> >> 
> >> Comment at: clang-tidy/readability/RedundantControlFlowCheck.cpp:60
> >> @@ +59,3 @@
> >> +  if (const auto *Return = dyn_cast(*last))
> >> +issueDiagnostic(Result, Block, Return->getSourceRange(),
> >> +RedundantReturnDiag);
> >> 
> >> It is the LLVM coding style (though I don't personally care for it), and
> you are right -- a clang-tidy check in the LLVM module wouldn't be amiss.
> :-)
> >
> > Isn't this:
> http://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-state
ments.html?
> 
> Yes, that seems to be it. Probably could have an alias in the LLVM
> umbrella that points here.

Sorry, I accidentally sent my first reply only to Kim Grasman.

No, google-readability-braces-around-statements doesn't apply LLVM coding
style rules.

google-readability-braces-around-statements tries to put braces around all
control structure bodies, but unfortunately creates errors.




It's a google-XXX check because that check is applying Google coding
style guidelines where braces are required around all control
structures.

LLVM uses a different coding style guideline for braces,
at least judging from code reviews I've had.  I just looked at
 and couldn't find any specific
mention of when braces are supposed to be used, I just keep getting dinged
in phabricator in what seems to me to be arbitrary ideas about when braces
should or shouldn't be used for single statement control structure bodies.

Instead of arbitrarily dinging code in reviews, I would prefer that
someone update the coding standards document to define what the rule
is and then we can write a clang-tidy checker that applies the rule.
-- 
"The Direct3D Graphics Pipeline" free book 
 The Computer Graphics Museum 
 The Terminals Wiki 
  Legalize Adulthood! (my blog) 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I agree it needs more testing.

I think also my current approach to newlines is overly aggressive and will 
result in more raw string literals than people would prefer.

It's really the Windows style path separators and regex ones that are not 
controversial transformations.


http://reviews.llvm.org/D16529



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

[PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.

Update `check_clang_tidy.py` to handle fixes applied to header files by adding 
`--header-filter` argument that:
- Passes `-header-filter` down to `clang-tidy`
- Copies named header to temporary directory where `clang-tidy` is run
- Performs `FileCheck` checks on the header for `CHECK-MESSAGES`
- Performs `FileCheck` checks on the header for `CHECK-FIXES`

Fixes PR25894

http://reviews.llvm.org/D16953

Files:
  clang-tidy/modernize/RedundantVoidArgCheck.cpp
  test/clang-tidy/check_clang_tidy.py
  test/clang-tidy/modernize-redundant-void-arg.cpp
  test/clang-tidy/modernize-redundant-void-arg.h

Index: test/clang-tidy/modernize-redundant-void-arg.h
===
--- /dev/null
+++ test/clang-tidy/modernize-redundant-void-arg.h
@@ -0,0 +1,8 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+
+extern int foo(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}extern int foo();{{$}}
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
Index: test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- test/clang-tidy/modernize-redundant-void-arg.cpp
+++ test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t
+// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t
+
+#include "modernize-redundant-void-arg.h"
 
 #define NULL 0
 
Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -25,6 +25,7 @@
 """
 
 import argparse
+import os
 import re
 import subprocess
 import sys
@@ -35,16 +36,98 @@
 f.write(text)
 f.truncate()
 
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+def clean_text(input_text):
+  return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text)
+
+
+def write_cleaned_header(input_file_path, temp_file_name, header_file_name):
+  src_path = os.path.join(os.path.dirname(input_file_path), header_file_name)
+  fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name)
+  with open(src_path, 'r') as input_file:
+cleaned_text = clean_text(input_file.read())
+  cleaned_path = fixed_path + '.orig'
+  write_file(cleaned_path, cleaned_text)
+  write_file(fixed_path, cleaned_text)
+  return cleaned_path, fixed_path, src_path
+
+
+def separate_output(clang_tidy_output, header_file_name, input_file_name):
+  input_file_name = os.path.basename(input_file_name)
+  input_file_output = ''
+  header_file_output = ''
+  input_messages = True
+  for line in clang_tidy_output.splitlines(True):
+if input_messages:
+  if header_file_name in line:
+input_messages = False
+header_file_output += line
+  else:
+input_file_output += line
+else:
+  if input_file_name in line:
+input_messages = True
+input_file_output += line
+  else:
+header_file_output += line
+  return header_file_output, input_file_output
+
+
+def try_run(args):
+  try:
+clang_tidy_output = \
+  subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+print('%s failed:\n%s' % (' '.join(args), e.output.decode()))
+raise
+  return clang_tidy_output
+
+
+def check_output_for_messages(messages_file, input_file_name):
+  try_run(
+['FileCheck', '-input-file=' + messages_file, input_file_name,
+   '-check-prefix=CHECK-MESSAGES',
+   '-implicit-check-not={{warning|error}}:'])
+
+
+def check_output_for_fixes(temp_file_name, input_file_name):
+  try_run(
+  ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
+   '-check-prefix=CHECK-FIXES', '-strict-whitespace'])
+
+
+def has_checks(input_text):
+  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
+  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  return has_check_fixes, has_check_messages
+
+
+def diff_source_files(original_file_name, temp_file_name):
+  try:
+diff_output = subprocess.check_output(
+  ['diff', '-u', original_file_name, temp_file_name],
+  stderr=subprocess.STDOUT)
+  except subprocess.CalledProcessError as e:
+diff_output = e.output
+  return diff_output
+
+
 def main():
   parser = argparse.ArgumentParser()
   parser.add_argument('

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood marked 5 inline comments as done.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:96
@@ +95,3 @@
+  std::string Delimiter;
+  for (int Counter = 0; containsDelimiter(Bytes, Delimiter); ++Counter) {
+Delimiter = (Counter == 0) ? "lit" : "lit" + std::to_string(Counter);

alexfh wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > Please address my comment above that relates to this code. Specifically, 
> > > I find this generic algorithm unnecessarily inefficient and too rigid, 
> > > i.e. one can't configure a custom set of delimiters that don't follow the 
> > >  pattern. I suggest using a list of pre-concatenated 
> > > pairs of strings (like `R"lit(` and `)lit"`).
> > Raw strings are new and not many people are using them because the don't 
> > have a good way to automatically convert disgusting quoted strings to raw 
> > strings.  So I don't think it is reasonable to draw conclusions by looking 
> > in existing code bases for raw strings.
> > 
> > We're having the same conversation we've had before.  I'm trying to do a 
> > basic check and get things working properly and the review comments are 
> > tending towards a desire for some kind of perfection.
> > 
> > I don't see why we have to make the perfect the enemy of the good.  Nothing 
> > you are suggesting **must** be present now in order for this check to 
> > function properly and reasonably.
> We're looking at this from different perspectives. From my point of view, 
> preventing a potentially wasteful code in ClangTidy checks before it's even 
> committed is much easier than tracking down performance issues when tens of 
> checks run on multiple machines analyzing millions lines of code. So I'm 
> asking to avoid writing code using string allocations and concatenations when 
> there are good alternatives. Apart from possible performance issues, in this 
> case the generic solution you suggest is targets extremely rare cases, while 
> most real-world situations can be handled with a fixed set of delimiters 
> (possibly, configured by the user, while I expect the preferences for the 
> choice of delimiters to be very different in different teams).
I believe we agree on the following:

  - In order to turn a non-raw string literal into a raw string literal I have 
to surround it with `R"(` and `)"`.
  - If the existing string literal contains `)"` already, then surrounding the 
literal with the above will result in a syntax error.  Therefore, every 
potential raw string literal will have to be searched at least once for this 
non-delimited form of raw string literal.
  - `clang-tidy` checks should never introduce syntax errors.

Therefore, I can either not transform the string literal if it contains `)"`, 
or I can come up with some delimited form of raw string literal where the 
delimiter does not appear in the string.  In order to not transform the 
literal, I first have to search for `)"` in order to know that it should not be 
transformed.  Therefore, the search for `)"` must be performed, no matter what 
algorithm is used to determine a delimited or non-delimited raw string literal.

Where we seem to disagree is what algorithm should be used to determine the 
delimiter when a delimited raw string literal is required.

My implementation is the minimum performance impact for the typical case where 
the string does not contain `)"`.

Now let's talk about the cases where searching the string repeatedly for a 
delimiter would be expensive.

First, the string literal will have to contain `)"`.

Second, the string literal would have to be lengthy for the delimiter searching 
to be expensive.  Most of the time lengthy string literals are when someone has 
transformed a text file into a giant string literal.  Such text files include 
newlines and in the latest version of the code, I've decided to skip any string 
literal containing a newline.  It simply results in too many strings being 
converted to raw string literals and obfuscates the newlines.  The one case 
where the embedded newlines makes sense is the case of the text file converted 
into a string literal.

Repeated searching of the string for the literal delimiter could be avoided by 
changing the routine to search for the delimiter.  If present, it could examine 
the characters following the literal to make the literal more unique and then 
continue searching from there to look for more occurrences of the extended 
delimiter.  It would proceed incrementally through the rest of the string to 
create a unique delimiter in a single pass through the string.  I think the 
resulting literals would be even more non-sensical than the current 
implementation, but it would result in a delimiter obtained by a single pass 
through the string.  It's a significantly more complicated implementation and 
results in an even weirder delimiter to handle a very edge case.

If I follow your suggested approach of using a fixed number of string 

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47157.
LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.

Update from review comments.

Added a bunch of test cases for non-printing characters, templates and macros.

Removed string literals containing newline (`\n`) from being considered for 
transformation to raw string literals.


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,122 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul{"goink\\\000"};
+char const *const Soh{"goink\\\001"};
+char const *const Stx{"goink\\\002"};
+char const *const Etx{"goink\\\003"};
+char const *const Enq{"goink\\\004"};
+char const *const Ack{"goink\\\005"};
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const NewLine{"goink\nfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const So{"goink\\\016"};
+char const *const Si{"goink\\\017"};
+char const *const Dle{"goink\\\020"};
+char const *const Dc1{"goink\\\021"};
+char const *const Dc2{"goink\\\022"};
+char const *const Dc3{"goink\\\023"};
+char const *const Dc4{"goink\\\024"};
+char const *const Nak{"goink\\\025"};
+char const *const Syn{"goink\\\026"};
+char const *const Etb{"goink\\\027"};
+char const *const Can{"goink\\\030"};
+char const *const Em{"goink\\\031"};
+char const *const Sub{"goink\\\032"};
+char const *const Esc{"goink\\\033"};
+char const *const Fs{"goink\\\034"};
+char const *const Gs{"goink\\\035"};
+char const *const Rs{"goink\\\036"};
+char const *const Us{"goink\\\037"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

It's been almost a week.  Is there some reason this hasn't been committed yet?


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/check_clang_tidy.py:122
@@ -40,2 +121,3 @@
   parser.add_argument('-resource-dir')
+  parser.add_argument('--header-filter')
   parser.add_argument('input_file_name')

alexfh wrote:
> There's no need for a separate argument for this. The script passes all 
> arguments after a `--` to clang-tidy, so you can just append `-- 
> -header-filter=... -- -std=c++11` to the RUN: line in the test.
This argument is the key to making everything work.  It triggers the copying of 
the header, the cleaning of the header of `CHECK-FIXES` and `CHECK-MESSAGES`, 
the diffing of the header, etc.

I originally had it like you suggested by trying to have the test file pass all 
the necessary extra arguments to clang-tidy, but it just isn't enough.  The 
script needs to do a bunch of extra work when headers are involved, so it 
seemed to make the most sense to pass the argument directly to the script so 
that it knows to do this extra work.

In other words, express the intent directly to the script instead of having the 
script infer intent by scraping through extra arguments.

Additionally, this preserves the behavior of eliminating duplicated arguments 
across all clang-tidy tests because the script will assume `-std=c++11` for 
`.cpp` files.


Comment at: test/clang-tidy/check_clang_tidy.py:152
@@ -68,4 +151,3 @@
 
-  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
-  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  has_check_fixes, has_check_messages = has_checks(input_text)
 

alexfh wrote:
> This function doesn't look like a particularly useful one: 5 lines of code 
> instead of 2 lines and an unclear interface (in which order do the two bools 
> come?). Please revert this part.
I initially had it doing the same checks on the header file as well and then it 
made more sense.  I dug myself into a dead-end on that series of changes, 
reverted and started over.

What I settled on here was the assumption that you won't have `CHECK-MESSAGES` 
or `CHECK-FIXES` in your header file, unless you also had them in the 
associated source file.  If that assumption is invalid, then I should call this 
function again for the header and change the tests to be asserting fixes in the 
source or header, messages in the source or header in various places.


http://reviews.llvm.org/D16953



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I wrote:

> Repeated searching of the string for the literal delimiter could be avoided 
> by changing the routine to search for the delimiter. If present, it could 
> examine the characters following the literal to make the literal more unique 
> and then continue searching from there to look for more occurrences of the 
> extended delimiter. It would proceed incrementally through the rest of the 
> string to create a unique delimiter in a single pass through the string. I 
> think the resulting literals would be even more non-sensical than the current 
> implementation, but it would result in a delimiter obtained by a single pass 
> through the string. It's a significantly more complicated implementation and 
> results in an even weirder delimiter to handle a very edge case.


Unfortunately in this paragraph I used the term "literal" in several places 
where I should have said "delimiter".  I hope that wasn't too confusing.  It 
should have read:

> Repeated searching of the string for the delimiter could be avoided by 
> changing the routine to search for the delimiter. If present, it could 
> examine the characters following the delimiter to make the delimiter more 
> unique and then continue searching from there to look for more occurrences of 
> the extended delimiter. It would proceed incrementally through the rest of 
> the string to create a unique delimiter in a single pass through the string. 
> I think the resulting delimiters would be even more non-sensical than the 
> current implementation, but it would result in a delimiter obtained by a 
> single pass through the string. It's a significantly more complicated 
> implementation and results in an even weirder delimiter to handle a very edge 
> case.



http://reviews.llvm.org/D16529



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In article ,

  Aaron Ballman  writes:

> aaron.ballman added a comment.

> 

> This is causing build bot failures:

> 

> http://bb.pgr.jp/builders/cmake-clang-tools-x86_64-linux/builds/23351

>  http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/492

>  http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/812

> 

> The failure is not obvious to me, and I don't have the opportunity to debug

>  locally right now, so I have reverted in r260097.


make check-all passes for me.  I don't see what the problem is in the logs
either.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

In article ,
Aaron Ballman  writes:

> aaron.ballman added a comment.
> 
> This is causing build bot failures:
> 
> http://bb.pgr.jp/builders/cmake-clang-tools-x86_64-linux/builds/23351
> http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/492
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/812
> 
> The failure is not obvious to me, and I don't have the opportunity to debug
> locally right now, so I have reverted in r260097.

make check-all passes for me.  I don't see what the problem is in the logs
either.
-- 
"The Direct3D Graphics Pipeline" free book 
 The Computer Graphics Museum 
 The Terminals Wiki 
  Legalize Adulthood! (my blog) 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood marked an inline comment as done.


Comment at: test/clang-tidy/check_clang_tidy.py:152
@@ -68,4 +151,3 @@
 
-  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
-  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  has_check_fixes, has_check_messages = has_checks(input_text)
 

LegalizeAdulthood wrote:
> alexfh wrote:
> > This function doesn't look like a particularly useful one: 5 lines of code 
> > instead of 2 lines and an unclear interface (in which order do the two 
> > bools come?). Please revert this part.
> I initially had it doing the same checks on the header file as well and then 
> it made more sense.  I dug myself into a dead-end on that series of changes, 
> reverted and started over.
> 
> What I settled on here was the assumption that you won't have 
> `CHECK-MESSAGES` or `CHECK-FIXES` in your header file, unless you also had 
> them in the associated source file.  If that assumption is invalid, then I 
> should call this function again for the header and change the tests to be 
> asserting fixes in the source or header, messages in the source or header in 
> various places.
IMO, "5 lines instead of 2 lines" isn't the kind of reasoning that is useful.  
Otherwise we would never do [[ 
https://www.industriallogic.com/xp/refactoring/composeMethod.html | Compose 
Method ]] on a long method since the act of breaking a long method down into a 
bunch of smaller methods necessarily introduces more lines of code.

This script originally was one giant function that did everything.  A bunch of 
things need to be done twice now: one for the source file and once for the 
header file.  Applying Compose Method Extracting results in extracting a group 
of smaller functions/methods.

Now the functions have a bunch of state that needs to be passed around between 
them and the functions might be more clearly expressed as methods on a class, 
but I didn't go that far with it.


http://reviews.llvm.org/D16953



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

Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47319.
LegalizeAdulthood added a comment.

Update from review comments


http://reviews.llvm.org/D16953

Files:
  clang-tidy/modernize/RedundantVoidArgCheck.cpp
  test/clang-tidy/check_clang_tidy.py
  test/clang-tidy/modernize-redundant-void-arg.cpp
  test/clang-tidy/modernize-redundant-void-arg.h

Index: test/clang-tidy/modernize-redundant-void-arg.h
===
--- /dev/null
+++ test/clang-tidy/modernize-redundant-void-arg.h
@@ -0,0 +1,8 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+
+extern int foo(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}extern int foo();{{$}}
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
Index: test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- test/clang-tidy/modernize-redundant-void-arg.cpp
+++ test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t
+// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t
+
+#include "modernize-redundant-void-arg.h"
 
 #define NULL 0
 
Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -25,6 +25,7 @@
 """
 
 import argparse
+import os
 import re
 import subprocess
 import sys
@@ -35,16 +36,98 @@
 f.write(text)
 f.truncate()
 
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+def clean_text(input_text):
+  return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text)
+
+
+def write_cleaned_header(input_file_path, temp_file_name, header_file_name):
+  src_path = os.path.join(os.path.dirname(input_file_path), header_file_name)
+  fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name)
+  with open(src_path, 'r') as input_file:
+cleaned_text = clean_text(input_file.read())
+  cleaned_path = fixed_path + '.orig'
+  write_file(cleaned_path, cleaned_text)
+  write_file(fixed_path, cleaned_text)
+  return cleaned_path, fixed_path, src_path
+
+
+def separate_output(clang_tidy_output, header_file_name, input_file_name):
+  input_file_name = os.path.basename(input_file_name)
+  input_file_output = ''
+  header_file_output = ''
+  input_messages = True
+  for line in clang_tidy_output.splitlines(True):
+if input_messages:
+  if header_file_name in line:
+input_messages = False
+header_file_output += line
+  else:
+input_file_output += line
+else:
+  if input_file_name in line:
+input_messages = True
+input_file_output += line
+  else:
+header_file_output += line
+  return header_file_output, input_file_output
+
+
+def try_run(args):
+  try:
+process_output = \
+  subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+print('%s failed:\n%s' % (' '.join(args), e.output.decode()))
+raise
+  return process_output
+
+
+def check_output_for_messages(messages_file, input_file_name):
+  try_run(
+['FileCheck', '-input-file=' + messages_file, input_file_name,
+   '-check-prefix=CHECK-MESSAGES',
+   '-implicit-check-not={{warning|error}}:'])
+
+
+def check_output_for_fixes(temp_file_name, input_file_name):
+  try_run(
+  ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
+   '-check-prefix=CHECK-FIXES', '-strict-whitespace'])
+
+
+def has_checks(input_text):
+  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
+  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  return has_check_fixes, has_check_messages
+
+
+def diff_source_files(original_file_name, temp_file_name):
+  try:
+diff_output = subprocess.check_output(
+  ['diff', '-u', original_file_name, temp_file_name],
+  stderr=subprocess.STDOUT)
+  except subprocess.CalledProcessError as e:
+diff_output = e.output
+  return diff_output
+
+
 def main():
   parser = argparse.ArgumentParser()
   parser.add_argument('-resource-dir')
+  parser.add_argument('--header-filter')
   parser.add_argument('input_file_name')
   parser.add_argument('check_name')
   parser.add_argument('temp_file_name')
 
   args, extra_args = parser.parse_known_args()
 
   resource_dir = args.resource_dir
+  header_filter = args.header_filter
   input_file_name = args.input_file_name
   check_name = args.check_name
   temp_file_name 

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47414.
LegalizeAdulthood added a comment.

Fixes StringRef problem that crashes tests in release builds only.


http://reviews.llvm.org/D16308

Files:
  clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tidy/readability/SimplifyBooleanExprCheck.h
  docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
  test/clang-tidy/readability-simplify-bool-expr.cpp

Index: test/clang-tidy/readability-simplify-bool-expr.cpp
===
--- test/clang-tidy/readability-simplify-bool-expr.cpp
+++ test/clang-tidy/readability-simplify-bool-expr.cpp
@@ -690,7 +690,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i & 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i & 1) != 0;{{$}}
 
 bool negated_if_implicit_bool_expr(int i) {
   if (i - 1) {
@@ -700,7 +700,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return !static_cast(i - 1);{{$}}
+// CHECK-FIXES: {{^}}  return (i - 1) == 0;{{$}}
 
 bool implicit_int(int i) {
   if (i) {
@@ -710,7 +710,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(i);{{$}}
+// CHECK-FIXES: {{^}}  return i != 0;{{$}}
 
 bool explicit_bool(bool b) {
   if (b) {
@@ -757,7 +757,7 @@
   }
 }
 // CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
-// CHECK-FIXES: {{^}}  return static_cast(~i);{{$}}
+// CHECK-FIXES: {{^}}  return ~i != 0;{{$}}
 
 bool logical_or(bool a, bool b) {
   if (a || b) {
@@ -830,7 +830,7 @@
   bool b = i ? true : false;
 }
 // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{.*}} in ternary expression result
-// CHECK-FIXES: bool b = static_cast(i);{{$}}
+// CHECK-FIXES: bool b = i != 0;{{$}}
 
 bool non_null_pointer_condition(int *p1) {
   if (p1) {
@@ -895,3 +895,38 @@
 // CHECK-MESSAGES: :[[@LINE-6]]:12: warning: {{.*}} in conditional return
 // CHECK-FIXES: {{^}}  if (b) {
 // CHECK-FIXES: {{^}}#define SOMETHING_WICKED false
+
+bool integer_not_zero(int i) {
+  if (i) {
+return false;
+  } else {
+return true;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: {{^}}  return i == 0;{{$}}
+
+class A {
+public:
+int m;
+};
+
+bool member_pointer_nullptr(int A::*p) {
+  if (p) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p != nullptr;{{$}}
+
+bool integer_member_implicit_cast(A *p) {
+  if (p->m) {
+return true;
+  } else {
+return false;
+  }
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:12: warning: {{.*}} in conditional return
+// CHECK-FIXES: return p->m != 0;{{$}}
Index: docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
===
--- docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
+++ docs/clang-tidy/checks/readability-simplify-boolean-expr.rst
@@ -35,11 +35,14 @@
   2. Negated applications of ``!`` are eliminated.
   3. Negated applications of comparison operators are changed to use the
  opposite condition.
-  4. Implicit conversions of pointer to ``bool`` are replaced with explicit
- comparisons to ``nullptr``.
+  4. Implicit conversions of pointers, including pointers to members, to
+ ``bool`` are replaced with explicit comparisons to ``nullptr`` in C++11
+  or ``NULL`` in C++98/03.
   5. Implicit casts to ``bool`` are replaced with explicit casts to ``bool``.
   6. Object expressions with ``explicit operator bool`` conversion operators
  are replaced with explicit casts to ``bool``.
+  7. Implicit conversions of integral types to ``bool`` are replaced with
+ explicit comparisons to ``0``.
 
 Examples:
   1. The ternary assignment ``bool b = (i < 0) ? true : false;`` has redundant
@@ -60,11 +63,11 @@
 
  The ternary assignment ``bool b = (i & 1) ? true : false;`` has an
  implicit conversion of ``i & 1`` to ``bool`` and becomes
- ``bool b = static_cast(i & 1);``.
+ ``bool b = (i & 1) != 0;``.
 
   5. The conditional return ``if (i & 1) return true; else return false;`` has
  an implicit conversion of an integer quantity ``i & 1`` to ``bool`` and
- becomes ``return static_cast(i & 1);``
+ becomes ``return (i & 1) != 0;``
 
   6. Given ``struct X { explicit operator bool(); };``, and an instance ``x`` of
  ``struct X``, the conditional return ``if (x) return true; return false;``
Index: clang-tidy/readability/SimplifyBooleanExprCheck.h
===
--- clang-tidy/readability/SimplifyBooleanExprCheck.h
+++ clang-tidy/readability/SimplifyBooleanExprCheck.h
@@ -19,75 +19,8 @@
 /// Looks for boolean expressions involving boolean constants and simplifies
 /// them to use

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article ,
Richard via cfe-commits  writes:

> make check-all passes for me.  I don't see what the problem is in the logs
> either.

OK, I found the problem.  Once again, StringRef bites me on the behind.

I have updated the review in phabricator with the corrected code.
(The review was closed, so apparently it didn't post to the list
when I updated it on phabricator?)

I surmised that the buildbots do a release build and when I did
check-clang-tools in release, I was able to reproduce the failure.
clang-tidy seg-faults.

It would be nice if we could get the build log output to record that
fact in the log.  I suspect it is the way that clang-tidy is invoked
by the check_clang_tidy.py python script.  Somehow the offending SEGV
message is suppressed.
-- 
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
 The Computer Graphics Museum <http://ComputerGraphicsMuseum.org>
 The Terminals Wiki <http://terminals.classiccmp.org>
  Legalize Adulthood! (my blog) <http://LegalizeAdulthood.wordpress.com>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: 
clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp:21-23
@@ +20,5 @@
+void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) {
+  auto Calc = expr(anyOf(binaryOperator(anyOf(
+ hasOperatorName("+"), hasOperatorName("-"),
+ hasOperatorName("*"), hasOperatorName("<<"))),
+ unaryOperator(hasOperatorName("~"))),

Sorry for the late observation, but why doesn't this check for `%` and `/` 
operators?


Repository:
  rL LLVM

http://reviews.llvm.org/D16310



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

Re: [PATCH] D16310: new clang-tidy checker misc-long-cast

[Richard via cfe-commits]

In article ,
Daniel Marjamäki  writes:

> > You can get overflow with / and %. Consider:
> >
> > int i = INT_MIN / -1; // similar for %
> 
> you are right, I did not think of that. 
> 
> imho overflow still seems unlikely for / and %.

I thought the whole point was to flag instances where the presence
of a cast indicated a suspicion of intent that wasn't being satisfied?

In other words, overflow may be unlikely but doesn't the presence of
the cast indicate the same suspicious intent?
-- 
"The Direct3D Graphics Pipeline" free book 
 The Computer Graphics Museum 
 The Terminals Wiki 
  Legalize Adulthood! (my blog) 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak?


http://reviews.llvm.org/D16953



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak?


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16308: clang-tidy Enhance readability-simplify-boolean-expr check to handle implicit conversions of integral types to bool and member pointers

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

If someone could review the changes and commit this corrected version for me, 
that would be great.

Patch by Richard Thomson.


http://reviews.llvm.org/D16308



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:97
@@ +96,3 @@
+}
+
+} // namespace

alexfh wrote:
> > I believe we agree on the following: ...
> 
> Yes.
> 
> > Where we seem to disagree is what algorithm should be used to determine the 
> > delimiter when a delimited raw string literal is required.
> 
> Pretty much so. IMO, we don't need a universal algorithm that will hardly 
> ever go further than the second iteration, and even in this case would 
> produce a result that's likely undesirable (as I said, `R"lit0()lit0"` is 
> not what I would want to see in my code).
> 
> The possibility of this code causing performance issues is probably not that 
> high, but I can imagine a code where this could be sub-optimal.
> 
> > My implementation is the minimum performance impact for the typical case 
> > where the string does not contain )".
> 
> One concern about this part is that it could issue 4 concatenation calls 
> fewer in case of an empty delimiter. This may already be handled well by the 
> `llvm::Twine` though.
> 
> Any code following potentially-problematic patterns might be fine on its own, 
> but it will attract unnecessary attention when reading code. High frequency 
> of such false alarms has non-trivial cost for code readers and makes it 
> harder to find actual problems.
> 
> So please, change the code to avoid these issues. Here's a possible 
> alternative:
> 
>   llvm::Optional asRawStringLiteral(const StringLiteral 
> *Literal) {
> const StringRef Bytes = Literal->getBytes();
> static const StringRef Delimiters[2][] =
>   {{"R\"(", ")\""}, {"R\"lit(", ")lit\""}, {"R\"str(", ")str\""},
>   /* add more different ones to make it unlikely to meet all of these in 
> a single literal in the wild */};
> for (const auto &Delim : Delimiters) {
>   if (Bytes.find(Delim[1]) != StringRef::npos)
> return (Delim[0] + Bytes + Delim[1]).str();
> }
> // Give up gracefully on literals having all our delimiters.
> return llvm::None;
>   }
I just don't see why replacing a general algorithm that always works with a 
bunch of hard-coded special cases is better.

You've reiterated your preference for one over the other, but since it simply a 
matter of preference, I don't see why this should be a requirement.

Working with the hard-coded list of delimiters is no more or less efficient 
than the general algorithm that I implemented, so efficiency is not the concern 
here.


Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:110
@@ +109,3 @@
+  if (const auto *Literal = Result.Nodes.getNodeAs("lit")) {
+if (isMacroExpansionLocation(Result, Literal->getLocStart()))
+  return;

alexfh wrote:
>   if (Literal->getLocStart().isMacroID())
> return;
I don't understand what this code is doing.  `isMacroID` is undocumented, so I 
don't know what it means for this function to return true without reverse 
engineering the implementation.


http://reviews.llvm.org/D16529



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47927.
LegalizeAdulthood marked 4 inline comments as done.
LegalizeAdulthood added a comment.

Update from review comments.
Avoid some string concatenation when delimiter is empty.


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,122 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul{"goink\\\000"};
+char const *const Soh{"goink\\\001"};
+char const *const Stx{"goink\\\002"};
+char const *const Etx{"goink\\\003"};
+char const *const Enq{"goink\\\004"};
+char const *const Ack{"goink\\\005"};
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const NewLine{"goink\nfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const So{"goink\\\016"};
+char const *const Si{"goink\\\017"};
+char const *const Dle{"goink\\\020"};
+char const *const Dc1{"goink\\\021"};
+char const *const Dc2{"goink\\\022"};
+char const *const Dc3{"goink\\\023"};
+char const *const Dc4{"goink\\\024"};
+char const *const Nak{"goink\\\025"};
+char const *const Syn{"goink\\\026"};
+char const *const Etb{"goink\\\027"};
+char const *const Can{"goink\\\030"};
+char const *const Em{"goink\\\031"};
+char const *const Sub{"goink\\\032"};
+char const *const Esc{"goink\\\033"};
+char const *const Fs{"goink\\\034"};
+char const *const Gs{"goink\\\035"};
+char const *const Rs{"goink\\\036"};
+char const *const Us{"goink\\\037"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const OctalPrintable{"\100\\"};
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47928.
LegalizeAdulthood added a comment.

Add FIXME for non-ASCII string literals.
Allow delimiter stem to be configured.
Avoid some string concatenation.


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal-delimiter.cpp
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,122 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul{"goink\\\000"};
+char const *const Soh{"goink\\\001"};
+char const *const Stx{"goink\\\002"};
+char const *const Etx{"goink\\\003"};
+char const *const Enq{"goink\\\004"};
+char const *const Ack{"goink\\\005"};
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const NewLine{"goink\nfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const So{"goink\\\016"};
+char const *const Si{"goink\\\017"};
+char const *const Dle{"goink\\\020"};
+char const *const Dc1{"goink\\\021"};
+char const *const Dc2{"goink\\\022"};
+char const *const Dc3{"goink\\\023"};
+char const *const Dc4{"goink\\\024"};
+char const *const Nak{"goink\\\025"};
+char const *const Syn{"goink\\\026"};
+char const *const Etb{"goink\\\027"};
+char const *const Can{"goink\\\030"};
+char const *const Em{"goink\\\031"};
+char const *const Sub{"goink\\\032"};
+char const *const Esc{"goink\\\033"};
+char const *const Fs{"goink\\\034"};
+char const *const Gs{"goink\\\035"};
+char const *const Rs{"goink\\\036"};
+char const *const Us{"goink\\\037"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const OctalPrintable{"\100\\"};
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw 

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 47929.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

Add FIXME for macro argument test case.


http://reviews.llvm.org/D16529

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/RawStringLiteralCheck.cpp
  clang-tidy/modernize/RawStringLiteralCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-raw-string-literal.rst
  test/clang-tidy/modernize-raw-string-literal-delimiter.cpp
  test/clang-tidy/modernize-raw-string-literal.cpp

Index: test/clang-tidy/modernize-raw-string-literal.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-raw-string-literal.cpp
@@ -0,0 +1,123 @@
+// RUN: %check_clang_tidy %s modernize-raw-string-literal %t
+
+char const *const BackSlash{"goink\\frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: escaped string literal can be written as a raw string literal [modernize-raw-string-literal]
+// CHECK-FIXES: {{^}}char const *const BackSlash{R"(goink\frob)"};{{$}}
+
+char const *const PlainLiteral("plain literal");
+
+// Non-printable ASCII characters.
+char const *const Nul{"goink\\\000"};
+char const *const Soh{"goink\\\001"};
+char const *const Stx{"goink\\\002"};
+char const *const Etx{"goink\\\003"};
+char const *const Enq{"goink\\\004"};
+char const *const Ack{"goink\\\005"};
+char const *const Bell{"goink\\\afrob"};
+char const *const BackSpace{"goink\\\bfrob"};
+char const *const HorizontalTab{"goink\\\tfrob"};
+char const *const NewLine{"goink\nfrob"};
+char const *const VerticalTab{"goink\\\vfrob"};
+char const *const FormFeed{"goink\\\ffrob"};
+char const *const CarraigeReturn{"goink\\\rfrob"};
+char const *const So{"goink\\\016"};
+char const *const Si{"goink\\\017"};
+char const *const Dle{"goink\\\020"};
+char const *const Dc1{"goink\\\021"};
+char const *const Dc2{"goink\\\022"};
+char const *const Dc3{"goink\\\023"};
+char const *const Dc4{"goink\\\024"};
+char const *const Nak{"goink\\\025"};
+char const *const Syn{"goink\\\026"};
+char const *const Etb{"goink\\\027"};
+char const *const Can{"goink\\\030"};
+char const *const Em{"goink\\\031"};
+char const *const Sub{"goink\\\032"};
+char const *const Esc{"goink\\\033"};
+char const *const Fs{"goink\\\034"};
+char const *const Gs{"goink\\\035"};
+char const *const Rs{"goink\\\036"};
+char const *const Us{"goink\\\037"};
+char const *const HexNonPrintable{"\\\x03"};
+char const *const Delete{"\\\177"};
+
+char const *const TrailingSpace{"A line \\with space. \n"};
+char const *const TrailingNewLine{"A single \\line.\n"};
+char const *const AlreadyRaw{R"(foobie\\bletch)"};
+char const *const UTF8Literal{u8"foobie\\bletch"};
+char const *const UTF8RawLiteral{u8R"(foobie\\bletch)"};
+char16_t const *const UTF16Literal{u"foobie\\bletch"};
+char16_t const *const UTF16RawLiteral{uR"(foobie\\bletch)"};
+char32_t const *const UTF32Literal{U"foobie\\bletch"};
+char32_t const *const UTF32RawLiteral{UR"(foobie\\bletch)"};
+wchar_t const *const WideLiteral{L"foobie\\bletch"};
+wchar_t const *const WideRawLiteral{LR"(foobie\\bletch)"};
+
+char const *const SingleQuote{"goink\'frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-XFIXES: {{^}}char const *const SingleQuote{R"(goink'frob)"};{{$}}
+
+char const *const DoubleQuote{"goink\"frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const DoubleQuote{R"(goink"frob)"};{{$}}
+
+char const *const QuestionMark{"goink\?frob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const QuestionMark{R"(goink?frob)"};{{$}}
+
+char const *const RegEx{"goink\\(one|two\\)\\?.*\\nfrob"};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const RegEx{R"(goink\(one|two\)\\\?.*\nfrob)"};{{$}}
+
+char const *const Path{"C:\\Program Files\\Vendor\\Application\\Application.exe"};
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const Path{R"(C:\Program Files\Vendor\Application\Application.exe)"};{{$}}
+
+char const *const ContainsSentinel{"who\\ops)\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:36: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsSentinel{R"lit(who\ops)")lit"};{{$}}
+
+char const *const ContainsDelim{"whoops)\")lit\""};
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: {{.*}} can be written as a raw string literal
+// CHECK-FIXES: {{^}}char const *const ContainsDelim{R"lit1(whoops)")lit")lit1"};{{$}}
+
+char const *const OctalPrintable{"\100\\"};
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: {{.*}} can be written as a raw string literal
+// CH

Re: [PATCH] D17244: [clang-tidy] readability-ternary-operator new check

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:10
@@ +9,3 @@
+And it gets transformed into::
+  (condition ? expression0 : expression1);
+

Why add the parentheses around the entire statement?  They seem to serve no 
purpose.

What happens if the `condition` is an expression with the same or lower 
precedence than the ternary operator?

Most people seem to agree that `(a || b) ? e1 : e2;` is more readable than `(a 
|| b ? e1 : e2);`


Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:13
@@ +12,3 @@
+Another situation when ternary operator would be suggested::
+  if (condition) return literal0; else return literal1;
+

[[ 
http://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html
 | readability-simplify-boolean-expr ]] does this when the return type is 
`bool` and the return statements are returning `true` and `false`.

It also handles conditional assignment via ternary operator when the value 
assigned is of type `bool` and the resulting expressions are boolean constants.

If this check also starts modifying the same ternary operators, then we can 
have two checks fighting to make changes if both checks are applied on the same 
run of `clang-tidy`.  I'm unsure how best to resolve the conflict.  I don't 
know what `clang-tidy` would do in such instances.


Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:18
@@ +17,3 @@
+
+The autofixes are only suggested when no comments would be lost.
+

Are you also checking for intermediate preprocessor lines?

```
if (cond) {
#define BLAH 1
  return BLAH;
} else {
  return 0;
}
```


Comment at: docs/clang-tidy/checks/readability-ternary-operator.rst:24
@@ +23,2 @@
+both comments will be lost, therefore there will be no autofix suggested for
+such case. If you want the check to perform an autofix just remove the 
comments.

A slightly better wording:

If you want the check to perform an autofix, either reposition or remove the 
comments.


Comment at: test/clang-tidy/readability-ternary-operator.cpp:3
@@ +2,3 @@
+
+bool Condition = true;
+int Variable = 0;

You don't have any test cases where the condition is an expression with 
operators.  Please add some tests for this, particularly operators of the same 
or lower precedence than the ternary operator.


http://reviews.llvm.org/D17244



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

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:15
@@ +14,3 @@
+
+  // warning here; p should be const
+  char f1(char *p) {

With pointers, there are always two layers of constness:


  - Whether the pointer itself is modified
  - Whether the data pointed to is modified

When you say "p should be const", I read that as the former.

I am pretty sure you intended the latter.  You should be more explicit in your 
documentation.  The ambiguity would have been resolved if you showed the 
"after" version of the code that would eliminate the warning.  This is 
semi-implied by your next example, but it's kinder to the reader to resolve 
this ambiguity immediately.




Comment at: docs/clang-tidy/checks/readability-non-const-parameter.rst:32
@@ +31,3 @@
+  // no warning; Technically, p can be const ("const struct S *p"). But making
+  // p const could be misleading. People might think that it's safe to pass
+  // const data to this function.

Here you are using "making p const" in the first sense I noted above.

It might help if you say "make `*p` const" when you mean that p points to 
constant data and use "making `p` const" when you mean that the pointer itself 
is unmodified.

There is also the wart in C++ that one can declare a function like this:

```
int f(char *p);
```

and define it like this:

```
int f(char *const p) {
  // ...
}
```

My sense is that this is pretty confusing to most programmers who really want 
the declaration and definition to be textually identical.


http://reviews.llvm.org/D15332



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

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

How hard is it to extend it to references?

Certainly the confusion about what is const is easier to resolve in the case of 
references because the references themselves are immutable.


http://reviews.llvm.org/D15332



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

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:3
@@ +2,3 @@
+
+// Currently the checker only warns about pointer arguments.
+//

danielmarjamaki wrote:
> LegalizeAdulthood wrote:
> > How hard is it to extend it to references?
> > 
> > Certainly the confusion about what is const is easier to resolve in the 
> > case of references because the references themselves are immutable.
> If a "int &" reference parameter is not written then probably it's better to 
> pass it by value than making it const. I would prefer that unless it has to 
> use "int &" to comply with some interface.
> 
> I realize that the same can be said about pointers. If there is a "int *p" 
> and you just read the value that p points at .. maybe sometimes it would be 
> preferable to pass it by value.
I thought the point of this check was to identify stuff passed by 
reference/pointer that could instead be passed by const reference/pointer.

Passing a read-only number type (`short`, `char`, `int`, `float`, `double`, 
etc.) parameter by pointer or by reference/pointer is a code smell, but this 
check isn't trying to identify that situation.

I'm more interested in things like:

```
void foo(std::string &s);
```

becoming

```
void foo(const std::string &s);
```

when `s` is treated as a read-only value within the implementation of `foo`.


http://reviews.llvm.org/D15332



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

Re: [PATCH] D16012: Carry raw string literal information through to the AST StringLiteral representation

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D16012#351827, @rsmith wrote:

> I'm definitely sympathetic to making `StringLiteralParser` expose information 
> [...]


I was unaware of this class; so far I have only studied the classes appearing 
in the AST.

I did notice that the AST shows string literals after concatenation (which 
makes perfect sense) and sometimes for refactoring you want to treat the 
individual string literal chunks separately.  My clang-tidy check for raw 
string literals is probably confused by literal concatenation in the AST, so I 
will go add some tests for those cases.

One area of clang-tidy/refactoring tooling code development that is pretty 
opaque at the moment is when you should dip down to relexing/reparsing the 
source text and when you use the AST.  So far most (all?) of the documentation 
on guiding developers to writing such tools talks almost exclusively about the 
AST.  Anything we can do to simplify lexing/parsing tasks when you need to dip 
lower than the AST would be most welcome.


http://reviews.llvm.org/D16012



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

Re: [PATCH] D10013: Refactor: Simplify boolean conditional return statements in lib/Driver

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I do not have commit access, so someone else will need to commit this.  Let me 
know if it needs rebasing.

Patch by Richard Thomson.


http://reviews.llvm.org/D10013



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

[PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.

This changeset improves check_clang_tidy.py to allow chnages made by a check to 
a header to be verified.

A new argument `--header-filter` allows the test file to specify the header 
that will be modified by the check.

`check_clang_tidy.py` will then:

- Copy the named header to the same temporary directory containing the source 
file
- Scrub `CHECK-FIXES` and `CHECK-FIXES` text from the copied header
- Run the `clang-tidy`  on the scrubbed source files with:
  - `-header-filter` and the name of the copied header file.
  -  `-I .` to specify the temporary directory in the include search path
- Verify messages and fixes on the source file.
- Verify messages and fixes on the header file.
- In the case of failure, report differences on the header and the source file.

http://reviews.llvm.org/D17482

Files:
  test/clang-tidy/check_clang_tidy.py
  test/clang-tidy/modernize-redundant-void-arg.cpp
  test/clang-tidy/modernize-redundant-void-arg.h

Index: test/clang-tidy/modernize-redundant-void-arg.h
===
--- /dev/null
+++ test/clang-tidy/modernize-redundant-void-arg.h
@@ -0,0 +1,8 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
+
+extern int foo(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg]
+// CHECK-FIXES: {{^}}extern int foo();{{$}}
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H
Index: test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- test/clang-tidy/modernize-redundant-void-arg.cpp
+++ test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t
+// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t
+
+#include "modernize-redundant-void-arg.h"
 
 #define NULL 0
 
Index: test/clang-tidy/check_clang_tidy.py
===
--- test/clang-tidy/check_clang_tidy.py
+++ test/clang-tidy/check_clang_tidy.py
@@ -25,6 +25,7 @@
 """
 
 import argparse
+import os
 import re
 import subprocess
 import sys
@@ -35,16 +36,98 @@
 f.write(text)
 f.truncate()
 
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+def clean_text(input_text):
+  return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text)
+
+
+def write_cleaned_header(input_file_path, temp_file_name, header_file_name):
+  src_path = os.path.join(os.path.dirname(input_file_path), header_file_name)
+  fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name)
+  with open(src_path, 'r') as input_file:
+cleaned_text = clean_text(input_file.read())
+  cleaned_path = fixed_path + '.orig'
+  write_file(cleaned_path, cleaned_text)
+  write_file(fixed_path, cleaned_text)
+  return cleaned_path, fixed_path, src_path
+
+
+def separate_output(clang_tidy_output, header_file_name, input_file_name):
+  input_file_name = os.path.basename(input_file_name)
+  input_file_output = ''
+  header_file_output = ''
+  input_messages = True
+  for line in clang_tidy_output.splitlines(True):
+if input_messages:
+  if header_file_name in line:
+input_messages = False
+header_file_output += line
+  else:
+input_file_output += line
+else:
+  if input_file_name in line:
+input_messages = True
+input_file_output += line
+  else:
+header_file_output += line
+  return header_file_output, input_file_output
+
+
+def try_run(args):
+  try:
+process_output = \
+  subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+print('%s failed:\n%s' % (' '.join(args), e.output.decode()))
+raise
+  return process_output
+
+
+def check_output_for_messages(messages_file, input_file_name):
+  try_run(
+['FileCheck', '-input-file=' + messages_file, input_file_name,
+   '-check-prefix=CHECK-MESSAGES',
+   '-implicit-check-not={{warning|error}}:'])
+
+
+def check_output_for_fixes(temp_file_name, input_file_name):
+  try_run(
+  ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
+   '-check-prefix=CHECK-FIXES', '-strict-whitespace'])
+
+
+def has_checks(input_text):
+  has_check_fixes = input_text.find('CHECK-FIXES') >= 0
+  has_check_messages = input_text.find('CHECK-MESSAGES') >= 0
+  return has_check_fixes, has_check_messages
+
+
+def diff_source_files(origina

Re: [PATCH] D17484: [clang-tidy] introduce modernize-deprecated-headers check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Other than a few nits, LGTM.



Comment at: clang-tidy/modernize/DeprecatedHeadersCheck.cpp:54
@@ +53,3 @@
+: Check(Check), LangOpts(LangOpts) {
+  CStyledHeaderToCxx["assert.h"] = "cassert";
+  CStyledHeaderToCxx["float.h"] = "cfloat";

Can't we use C++11 brace initialization here instead of repeated assignments?


Comment at: docs/clang-tidy/checks/modernize-deprecated-headers.rst:33
@@ +32,3 @@
+
+The following headers were deprecated before C++ 11:
+

Can we sort these lists of includes?  Otherwise it's hard to find a specific 
include in the list.


Comment at: test/clang-tidy/modernize-deprecated-headers-cxx11.cpp:3
@@ +2,3 @@
+
+#include "assert.h"
+#include "float.h"

Can we sort the includes in the test files too please?


http://reviews.llvm.org/D17484



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

Re: [PATCH] D17484: [clang-tidy] introduce modernize-deprecated-headers check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17484#358617, @alexfh wrote:

> Thank you for this check! Mostly looks good, but there are a number of style 
> nits.
>
> The most important question to this check is that the standard doesn't 
> guarantee that the C++ headers declare all the same functions **in the global 
> namespace** (at least, this is how I understand the section of the standard 
> you quoted).


Oh crap, I totally forgot about this.  Yes, I believe the weasel wording in the 
standard is that an implementation may put the symbols in the global namespace 
as well as inside `std` when the C++ header is used, but is not required to put 
them in the global namespace.  They are required to put them in namespace `std` 
when the C++ header form is used.

So if you switch to C++ headers, you code may still compile, but it is 
implementation dependent.

If you use the C headers, you can't prefix symbols with `std` because they 
won't be there.

In other discussions of this topic, it was considered best to make both changes 
at the same time: switch to the C++ header and decorate the symbols with `std` 
prefix.

It would be reasonable for this check to insert a `using namespace std;` at the 
top of the translation unit after all the `#include` lines as a means of 
preserving meaning without trying to identify every symbol that needs `std` on 
it.


http://reviews.llvm.org/D17484



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

Re: [PATCH] D17446: ASTMatchers: add new statement/decl matchers

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/LibASTMatchersReference.html:512
@@ +511,3 @@
+MatcherStmt>addrLabelExprMatcherAddrLabelExpr>...
+Matches address of 
label statements (GNU extension).
+

Can we put `gnu` in the name of matchers that only match GNU extensions?


Comment at: docs/LibASTMatchersReference.html:544
@@ +543,3 @@
+MatcherStmt>atomicExprMatcherAtomicExpr>...
+Matches atomic builtins.
+

Please provide an example here.


Comment at: docs/LibASTMatchersReference.html:549
@@ +548,3 @@
+MatcherStmt>binaryConditionalOperatorMatcherBinaryConditionalOperator>...
+Matches 
binary conditional operator expressions (GNU extension).
+

See above re: gnu specific extensions.  When I read the name of this matcher, I 
expected it to be matching `==`, `!=`, `<=`, etc.


Comment at: docs/LibASTMatchersReference.html:1120
@@ +1119,3 @@
+MatcherStmt>opaqueValueExprMatcherOpaqueValueExpr>...
+Matches opaque 
value expressions.
+

The docs say that OpaqueValueExpr doesn't correspond to concrete syntax, so why 
would we have a matcher for it?


Comment at: docs/LibASTMatchersReference.html:1140
@@ +1139,3 @@
+Given
+  template class X { void f() { X x(*this); } };
+parenListExpr()

Can we have a simpler example?  For instance, I can't see that `ParenListExpr` 
has anything to do with templates.


Comment at: docs/LibASTMatchersReference.html:1149
@@ +1148,3 @@
+
+Example: Matches __func__)
+  printf("%s", __func__);

Does it actually match the closing paren?


Comment at: docs/LibASTMatchersReference.html:1175
@@ +1174,3 @@
+MatcherStmt>stmtExprMatcherStmtExpr>...
+Matches GNU statement 
expression.
+

Ditto re: gnu extension


Comment at: docs/LibASTMatchersReference.html:1752
@@ +1751,3 @@
+MatcherCXXConstructExpr>requiresZeroInitialization
+Matches 
a constructor call expression which requires
+zero initialization.

Please provide an example.


http://reviews.llvm.org/D17446



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

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you add a synopsis of this new check to `docs/ReleaseNotes.rst` please?


Repository:
  rL LLVM

http://reviews.llvm.org/D17811



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

Re: [PATCH] D18408: readability check for const params in declarations

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

You can submit the release notes changes in a new patch.


Repository:
  rL LLVM

http://reviews.llvm.org/D18408



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

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Please summarize this check in `docs/ReleaseNotes.rst`.

There is a pending review that will stub this out automatically, but it hasn't 
been approved yet


http://reviews.llvm.org/D18265



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Please summarize the improvements in `docs/ReleaseNotes.rst` in the clang tidy 
section.


http://reviews.llvm.org/D18396



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

Re: [PATCH] D18136: boost-use-to-string check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Please summarize this check in `docs/ReleaseNotes.rst` in the clang-tidy 
section.


http://reviews.llvm.org/D18136



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

Re: [PATCH] D18408: readability check for const params in declarations

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Maybe you need to rebase first?  I haven't used arc.


Repository:
  rL LLVM

http://reviews.llvm.org/D18408



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

Re: [PATCH] D18457: [clang-tidy] Add a new checker to detect missing comma in initializer list.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you summarize this check in `docs/ReleaseNotes.rst` in the clang-tidy 
section, please?


http://reviews.llvm.org/D18457



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

Re: [PATCH] D18475: [clang-tidy] Add more detection rules for redundant c_str calls.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you summarize the improvements in `docs/ReleaseNotes.rst` please?


http://reviews.llvm.org/D18475



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

Re: [PATCH] D18582: [Clang-tidy] Update release notes with list of checks added since 3.8

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/ReleaseNotes.rst:143
@@ -71,3 +142,3 @@
 Improvements to ``modularize``
 ^^
 

The formatting here has been changed on trunk.  Please rebase with a full file 
context so we can see the whole file in phabricator.


Repository:
  rL LLVM

http://reviews.llvm.org/D18582



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

Re: [PATCH] D18584: Complete support for C++ Core Guidelines Type.6: Always initialize a member variable.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

Can you summarize the improvements in `docs/ReleaseNotes.rst`, please?


http://reviews.llvm.org/D18584



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

Re: [PATCH] D18509: clang-tidy: add_new_check.py stubs out release notes

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/add_new_check.py:283
@@ +282,3 @@
+  elif not clang_tidy_heading_found:
+if line.startswith('^^'):
+  clang_tidy_heading_found = True

Alex changed the formatting, so this will need to be rebased.


http://reviews.llvm.org/D18509



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I'm sorry to be such a nag and I know you're busy, but

This changeset has been held up for a month and a half. (6 weeks since 
originally posted in http://reviews.llvm.org/D16953)

The change isn't that complicated and there haven't really been any comments 
requiring action on my part, so I don't understand why this is taking so long.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D18608: note for top-level consts in function decls tidy

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/ReleaseNotes.rst:153-154
@@ -152,2 +152,4 @@
 
+  * `readability-avoid-const-params-in-decls
+
`_
   * `readability-identifier-naming

Was this added in 3.8 or is it being added in 3.9?

That URL gives me 404 not found.

If it is new to 3.9, it should be farther up in the file.


http://reviews.llvm.org/D18608



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

Re: [PATCH] D18582: [Clang-tidy] Update release notes with list of checks added since 3.8

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/ReleaseNotes.rst:143
@@ -71,3 +142,3 @@
 Improvements to ``modularize``
 ^^
 

LegalizeAdulthood wrote:
> The formatting here has been changed on trunk.  Please rebase with a full 
> file context so we can see the whole file in phabricator.
Alex changed:

```
Improvements to ``modularize``
^^
```
to
```
Improvements to modularize
--
```
(See [[ 
https://raw.githubusercontent.com/llvm-mirror/clang-tools-extra/master/docs/ReleaseNotes.rst
 | raw version on github mirror ]])

and your diff is against the old version, so you will want to rebase the 
changeset and post a full context diff when you update this review.


Repository:
  rL LLVM

http://reviews.llvm.org/D18582



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

Re: [PATCH] D18608: note for top-level consts in function decls tidy

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: docs/ReleaseNotes.rst:153-154
@@ -152,2 +152,4 @@
 
+  * `readability-avoid-const-params-in-decls
+
`_
   * `readability-identifier-naming

fowles wrote:
> LegalizeAdulthood wrote:
> > Was this added in 3.8 or is it being added in 3.9?
> > 
> > That URL gives me 404 not found.
> > 
> > If it is new to 3.9, it should be farther up in the file.
> I don't know.  It was committed yesterday.
Hrm.  I see the page describing it here:
http://clang.llvm.org/extra/clang-tidy/checks/readability-avoid-const-params-in-decls.html
but I think those docs are continuously rebuilt from trunk.

The release specific URL you added does not work and if you look in the base 
directory for that URL:
http://llvm.org/releases/3.8.0/tools/clang/tools/extra/docs/clang-tidy/checks/
you will not find a file for readability-avoid-const-params-in-decls

I suspect it means that the documentation was added since the 3.8 release and 
therefore the check isn't actually available in the 3.8 release either.  Yep, 
looking at the github mirror for branch release_38, this check isn't present.  
See the [[ 
https://github.com/llvm-mirror/clang-tools-extra/tree/release_38/clang-tidy/readability
 | mirror on github ]] to verify this.

Therefore, I conclude that the check should be listed in the what's new for 3.9 
section and not what was added between 3.7 and 3.8.


http://reviews.llvm.org/D18608



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

Re: [PATCH] D18509: clang-tidy: add_new_check.py stubs out release notes

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D18509#388071, @hintonda wrote:

> With this change, won't you need to update 2 different repos: clang and 
> extra?  Is it possible to update both with a single Phabricator patch?


This is updating the release notes in the extra repository.  (The release notes 
are new to this repository; you may not have noticed them being added.)


http://reviews.llvm.org/D18509



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

As it stands currently, you can't commit a header with `CHECK-MESSAGES` and 
`CHECK-FIXES` lines and have them verified.

That's the whole point of this changeset.

Currently you have to do something very hacky in order to verify changes made 
to headers.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17482#390237, @alexfh wrote:

> My main concern is that this change makes the already complicated and 
> unobvious testing mechanism [...]


The complexity and obtuseness of the existing testing mechanism is unrelated to 
this changeset.  This changeset doesn't fundamentally change the testing 
mechanism.

> even more complicated and even less obvious for a very little (as I see it 
> now) benefit.


The benefit is that you would now have a consistent mechanism for verifying 
changes made to headers.

> The added functionality supports a very limited use case (a single header)


You have to start somewhere.  Currently there is no mechanism provided at all.  
Saying that this mechanism is not acceptable because it doesn't handle 
arbitrarily complex generalized checking is making the perfect the enemy of the 
good and isn't a reason for preventing this change from being accepted IMO.

> and it does it in a rather hacky way (the use of the new --header-filter flag 
> is not self-documenting and the overall behavior seems "magical" at the first 
> glance).


None of the mechanisms in the testing of these files is self-documenting.  If 
that is the criteria by which all changes are to be measured, then the entire 
existing system has to be thrown out.  Again, this feels like making the 
perfect the enemy of the good.  The behavior for validating header files is the 
same as the existing behavior for validating source files.  They are copied, 
filtered, processed and the processed results are analyzed.  Discrepencies are 
shown as diffs against the original source files.

> There's also an outstanding comment that you didn't seem to have addressed 
> yet.


Which specific comment are you referring to?  Because, just like this comment, 
there's a bunch of discussion in very general terms without specific complaints 
against specific pieces of code that are preventing it from being committed.

> In http://reviews.llvm.org/D17482#378685, @LegalizeAdulthood wrote:

> 

> > In the context of a clang-tidy check, what would be gained by verifying 
> > changes made to multiple headers that isn't already tested by verifying 
> > changes made to a single header?

> 


You haven't answered this question yet.  The existing test mechanism verifies 
only a single source file at a time; I see no reason why verifying a single 
header is such a great imposition of constraints that would prevent existing 
checks from being enhanced to verify their changes on header files.  However, 
should that arise in the future it is a relatively small change to add 
additional header processing to the script.  Again, let's not make the perfect 
the enemy of the good.  There is no reason we cannot improve the code in steps, 
on demand, as the need arises.  This is the essence of agile development.  
YAGNI 

> > At least two of the checks I've already added have complaints that they 
> > don't work on headers.

> 

> 

> That's because you used `isExpansionInMainFile`, which is just not needed 
> there


...and that was because I couldn't write an automated test against header files 
to verify the changes made there.  The whole point of THIS changeset was to 
give me a mechanism for verifying the fixes in a header so that I could address 
those issues in the bug database.

But instead of getting on with fixing it, I've spent 6 weeks waiting for this 
changeset to be reviewed and that discussion has prevented an advancement of 
functionality in the testing framework.

> > While this change was waiting to be reviewed, another check was added that 
> > made changes to headers but had no way to verify them.

> 

> 

> Which check do you mean? Does it need a special treatment of headers or is it 
> just able to make fixes in headers as most other checks?


I already quoted the check in question earlier in this thread; please review 
those messages.


http://reviews.llvm.org/D17482



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

Re: [clang-tools-extra] r264563 - clang-tidy: Fix broken buildbot

[Richard via cfe-commits]

In article ,
Alexander Kornienko  writes:
> 
> On Mon, Mar 28, 2016 at 6:15 AM, Richard Thomson via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
> 
> > Author: legalize
> > Date: Sun Mar 27 23:15:41 2016
> > New Revision: 264563
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=264563&view=rev
> > Log:
> > clang-tidy: Fix broken buildbot
> >
> > VS 2013 does not support char16_t or char32_t

> I'm not sure I understand what this test has to do with VS2013.

I incorrectly stated it as VS 2013 and not the Windows buildbot
configuration as being the root cause.

If you look at the failed build message, it is clearly saying "error:
unknown type name 'char16_t'" and "error: unknown type name 'char32_t'".


I could either do something that would fix the build quickly (which is
what I chose to do), or I could reverse out the entire changeset.

I chose the former as the best means of getting the builds green
again.  The existing raw string literal check doesn't yet process
character literals other than ASCII literals, so there is nothing lost
in commenting out these lines of the test file.

> Clang-tidy
> should be able to parse this code, and if it doesn't (e.g. due to
> -fms-compatibility being turned on by default on windows), we should put
> unsupported constructs under an appropriate #ifdef, not comment them out
> completely.

That is why I added a TODO comment instead of just removing the lines.

Feel free to patch it such that it does some kind of CMake based
feature check for char16_t and char32_t and then configures a header
file with HAS_xxx style check macros, include that generated header
file in the test sources and then use the preprocessor to conditionally
include the lines in the test file based on compiler support.

To me, that is a significant chunk of work, which is why I opted
instead for commenting out these (not too important yet) lines of test
code and add a TODO comment.  That TODO can be addressed when the
check is expanded to handle non-ASCII string literals.
-- 
"The Direct3D Graphics Pipeline" free book 
 The Computer Graphics Museum 
 The Terminals Wiki 
  Legalize Adulthood! (my blog) 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18745: [clang-tidy] Adds modernize-use-bool-literals check.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/clang-tidy/checks/modernize-use-bool-literals.rst:17
@@ +16,1 @@
+  std::ios_base::sync_with_stdio(false);
\ No newline at end of file


Please ensure the file ends with a newline.


Comment at: test/clang-tidy/modernize-use-bool-literals.cpp:3
@@ +2,3 @@
+
+bool bar1 = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: implicitly converting integer 
literal to bool, use bool literal instead [modernize-use-bool-literals]

There is more initialization syntax than just assignment.  Here are the ones I 
can think of:

What happens when I write `bool foo{1};` or `bool foo(1);`?

What happens when I write:

```
class foo {
public:
  foo() : b(0) {}
  foo(int) : b{0} {}
private:
  bool b;
  bool c{0};
  bool d(0);
  bool e = 0;
};
```

Should we warn an a `bool` is initialized by a call to a `constexpr` function 
that returns an integral type?


http://reviews.llvm.org/D18745



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

Re: [PATCH] D18783: [clang-tidy] add new checker for string literal with NUL character.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

There are some APIs where a series of strings are passed as a single `char *` 
with NUL separators between the strings and the last string is indicated by two 
NUL characters in a row.  For instance, see the MSDN documentation 

 for the `lpDependencies` argument to `CreateService` 
.
  Such strings literals in the code look like:

  LPCTSTR dependencies = _T("one\0two\0three\0four\0");

I don't see any annotation on this argument in `` where this function 
is declared, so there's no hint from the header about this argument.  In this 
case, the biggest mistake people make is omitting the final NUL character, 
thinking that the `\0` acts as a string separator and not a terminator:

  LPCTSTR dependencies = _T("one\0two\0three\0four");

If you're lucky this results in a crash right away.  If you're unlucky the 
compiler placed this constant in an area of memory with zero padding and it 
works by accident on your machine until it ships to a customer and then breaks 
on their machine mysteriously.

I was never a fan of these magic string array arguments and they mostly appear 
in the older portions of the Win32 API, but they are there nonetheless.  I 
would hate for this check to signal a bunch of false positives on such strings.


http://reviews.llvm.org/D18783



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

Re: [PATCH] D18852: [clang-tidy] fix a crash with -fdelayed-template-parsing in UnnecessaryValueParamCheck.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.
LegalizeAdulthood added a comment.

That we've had to fix this twice now tells me that our collective memory is 
forgetful :).

We need to collect the community wisdom for `clang-tidy` gotchas somewhere in 
the docs...


http://reviews.llvm.org/D18852



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

Re: [PATCH] D16535: [clang-tidy] Check to find unintended semicolons that changes the semantics.

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: docs/clang-tidy/checks/misc-suspicious-semicolon.rst:48-49
@@ +47,4 @@
+
+while(readWhitespace());
+  Token t = readNextToken();
+

In this documentation, can we please format code so that control structures 
don't look like function calls?  By this I mean write:
```
if (x >=y);
  x -= y;

while (readwhitespace());
  Token t = readNextToken();
```
instead of
```
if(x >=y);
  x -= y;

while(readwhitespace());
  Token t = readNextToken();
```



Repository:
  rL LLVM

http://reviews.llvm.org/D16535



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak


http://reviews.llvm.org/D17482



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak.  This has been waiting on review for over two weeks


http://reviews.llvm.org/D16529



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

Re: [PATCH] D10013: Refactor: Simplify boolean conditional return statements in lib/Driver

[Richard via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.

Discarding due to inaction by reviewers.


http://reviews.llvm.org/D10013



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

Re: [PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I need to update from review comments and upload a new diff.


http://reviews.llvm.org/D7982



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

Re: [PATCH] D16529: [clang-tidy] Add modernize-raw-string-literal check

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I do not have commit access, so I will need someone to commit this for me.

Patch by Richard Thomson

thanks.



Comment at: clang-tidy/modernize/RawStringLiteralCheck.cpp:82
@@ +81,3 @@
+? std::string{R"lit()")lit"}
+: (")" + Delimiter + R"(")")) != StringRef::npos;
+}

aaron.ballman wrote:
> This is a wonderful demonstration of why I hate raw string literals on 
> shorter strings. I had to stare at that raw string for quite some time to 
> figure it out. :-(
I used a raw string literal here because that's what this check would have 
recommended on this code :-).

However, your feedback is useful.  If I subsequently enhance this check with a 
parameter that says the minimum length a string literal must have before it is 
to be transformed into a raw string literal and the default value for that 
parameter is something like 5, would that address your concern?


http://reviews.llvm.org/D16529



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Another week without reviews...

These changes were already split from a previous changeset without being 
reviewed properly.

Squeak squeak squeak.


http://reviews.llvm.org/D17482



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

Re: [PATCH] D18136: boost-use-to-string check

[Richard via cfe-commits]

LegalizeAdulthood added a subscriber: LegalizeAdulthood.


Comment at: clang-tidy/boost/BoostTidyModule.cpp:21
@@ -19,3 +20,3 @@
 
 class ModernizeModule : public ClangTidyModule {
 public:

This should be `BoostModule`, not `ModernizeModule`.


Comment at: clang-tidy/boost/UseToStringCheck.cpp:24
@@ +23,3 @@
+  // returning type of className.
+  auto getMatcher = [](std::string className) {
+return callExpr(

`getMatcher` doesn't reveal the intent here; the name should reveal what is 
matched without me having to reverse engineer that information from it's 
defintiion.


Comment at: clang-tidy/boost/UseToStringCheck.cpp:33
@@ +32,3 @@
+  Finder->addMatcher(
+  getMatcher("class std::__cxx11::basic_string").bind("to_string"),
+  this);

Why do we need to match something that is implementation specific?  This seems 
really fragile.


Comment at: clang-tidy/boost/UseToStringCheck.cpp:40
@@ +39,3 @@
+  Finder->addMatcher(
+  getMatcher("class std::__cxx11::basic_string").bind("to_wstring"),
+  this);

Ditto


Comment at: clang-tidy/boost/UseToStringCheck.cpp:55-58
@@ +54,6 @@
+
+static const std::string toStringMSG =
+"use std::to_string instead of boost::lexical_cast";
+static const std::string toWStringMSG =
+"use std::to_wstring instead of boost::lexical_cast";
+

Why do we need variables for these strings?  They are used only once and the 
introduced variable name doesn't reveal more intent.


Comment at: docs/clang-tidy/checks/list.rst:90
@@ -88,2 +89,3 @@
modernize-use-override
+   modernize-use-using
performance-faster-string-find

Why was this added?  It seems unrelated to this changeset.


Comment at: test/clang-tidy/boost-use-to-string.cpp:4-10
@@ +3,9 @@
+
+namespace std {
+
+template  class basic_string {};
+
+using string = basic_string;
+using wstring = basic_string;
+}
+

Isn't there an existing stub header file providing std::{w,}string?


Comment at: test/clang-tidy/boost-use-to-string.cpp:76-81
@@ +75,8 @@
+// CHECK-FIXES: fun(std::to_string(f));
+  fun(boost::lexical_cast(g));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_string instead of 
boost::lexical_cast [boost-use-to-string]
+// CHECK-FIXES: fun(std::to_string(g));
+  fun(boost::lexical_cast(h));
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::to_string instead of 
boost::lexical_cast [boost-use-to-string]
+// CHECK-FIXES: fun(std::to_string(h));
+  fun(boost::lexical_cast(i));

Do we know that the semantics are the same for floating-point types?


http://reviews.llvm.org/D18136



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

Re: [PATCH] D18191: [clang-tidy] Add check for function parameters that are const& to builtin types

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D18191#377686, @sdowney wrote:

> In http://reviews.llvm.org/D18191#376471, @LegalizeAdulthood wrote:
>
> > There is utility in the definition of a function in saying that an argument 
> > is `const int i` instead of `int i`.  The const-ness declares the intent 
> > that this local variable is not going to be modified.  However, there is 
> > that oddity in C++ that allows a declaration to say `void f(int i);` and 
> > the implementation to say `void f(const int i) { ... }`.
> >
> > I think I would like the fixit to preserve the const-ness of the argument 
> > while still stripping it of it's reference.
>
>
> The usual pattern for that is to have the definition use const, and the 
> declaration not, since it's an implementation detail if the passed parameter 
> is modified. And, unfortunately, I'm stuck with one compiler that mangles 
> those two differently, causing link errors.
>
> Having void foo(const int i) in a declaration makes me suspicious, since the 
> const is meaningless.
>
> I could make this an option? If the option is set, emit 'const type' if the 
> Decl hasBody?


It seems reasonable to do this as a refinement after this check is in place.


http://reviews.llvm.org/D18191



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

Re: [PATCH] D18263: [clang-tools-extra] Add release notes documentation

[Richard via cfe-commits]

LegalizeAdulthood updated this revision to Diff 51102.
LegalizeAdulthood marked 2 inline comments as done.
LegalizeAdulthood added a comment.

Update from review comments.

I do not have commit access.

Patch by Richard Thomson


http://reviews.llvm.org/D18263

Files:
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/index.rst

Index: docs/index.rst
===
--- docs/index.rst
+++ docs/index.rst
@@ -10,6 +10,10 @@
 Welcome to the clang-tools-extra project which contains extra tools built using
 Clang's tooling API's.
 
+.. toctree::
+   :maxdepth: 1
+
+   ReleaseNotes
 
 Contents
 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
performance-faster-string-find
performance-for-range-copy
performance-implicit-cast-in-loop
+   performance-unnecessary-copy-initialization
readability-braces-around-statements
readability-container-size-empty
readability-else-after-return
Index: docs/ReleaseNotes.rst
===
--- /dev/null
+++ docs/ReleaseNotes.rst
@@ -0,0 +1,71 @@
+=
+Extra Clang Tools 3.9 (In-Progress) Release Notes
+=
+
+.. contents::
+   :local:
+   :depth: 2
+
+Written by the `LLVM Team `_
+
+.. warning::
+
+   These are in-progress notes for the upcoming Clang 3.9 release. You may
+   prefer the `Clang 3.8 Release Notes
+   `_.
+
+Introduction
+
+
+This document contains the release notes for the Extra Clang Tools, part of the
+Clang release 3.9.  Here we describe the status of the Extra Clang Tools in some
+detail, including major improvements from the previous release and new feature
+work. For the general Clang release notes, see `the Clang documentation
+`_.  All LLVM
+releases may be downloaded from the `LLVM releases web
+site `_.
+
+For more information about Clang or LLVM, including information about
+the latest release, please see the `Clang Web Site `_ or
+the `LLVM Web Site `_.
+
+Note that if you are reading this file from a Subversion checkout or the
+main Clang web page, this document applies to the *next* release, not
+the current one. To see the release notes for a specific release, please
+see the `releases page `_.
+
+What's New in Extra Clang Tools 3.9?
+
+
+Some of the major new features and improvements to Extra Clang Tools are listed
+here. Generic improvements to Extra Clang Tools as a whole or to its underlying
+infrastructure are described first, followed by tool-specific sections.
+
+Major New Features
+--
+
+- Feature1...
+
+Improvements to ``clang-query``
+^^^
+
+The improvements are...
+
+Improvements to ``clang-rename``
+
+
+The improvements are...
+
+Improvements to ``clang-tidy``
+^^
+
+``clang-tidy``'s checks are constantly being improved to catch more issues,
+explain them more clearly, and provide more accurate fix-its for the issues
+identified.  The improvements since the 3.8 release include:
+
+-  ...
+
+Improvements to ``modularize``
+^^
+
+The improvements are...
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

test/clang-tidy/misc-unused-parameters.cpp currently does a hack to verify 
headers by committing the manually fixed header and diffing it:

  // RUN: echo "static void staticFunctionHeader(int i) {}" > %T/header.h
  // RUN: echo "static void staticFunctionHeader(int  /*i*/) {}" > 
%T/header-fixed.h
  // RUN: %check_clang_tidy %s misc-unused-parameters %t -- -header-filter='.*' 
-- -std=c++11 -fno-delayed-template-parsing
  // RUN: diff %T/header.h %T/header-fixed.h
  
  #include "header.h"
  // CHECK-MESSAGES: header.h:1:38: warning

I think this would be much cleaner with `header.h` as a regularly comitted file 
with `CHECK-MESSAGES` and `CHECK-FIXES` lines in that file.


http://reviews.llvm.org/D17482



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