date:20181210

[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.

2018-12-10 Thread Clement Courbet via Phabricator via cfe-commits

courbet added a comment.

Thanks.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55270/new/

https://reviews.llvm.org/D55270



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

r348741 - [Sema] Further improvements to to static_assert diagnostics.

2018-12-10 Thread Clement Courbet via cfe-commits

Author: courbet
Date: Mon Dec 10 00:19:38 2018
New Revision: 348741

URL: http://llvm.org/viewvc/llvm-project?rev=348741&view=rev
Log:
[Sema] Further improvements to to static_assert diagnostics.

Summary:
We're now handling cases like `static_assert(!expr)` and
static_assert(!(expr))`.

Reviewers: aaron.ballman, Quuxplusone

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D55270

Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Sema/SemaTemplate.cpp
cfe/trunk/test/PCH/cxx-static_assert.cpp
cfe/trunk/test/Sema/static-assert.c
cfe/trunk/test/SemaCXX/static-assert-cxx17.cpp
cfe/trunk/test/SemaCXX/static-assert.cpp

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=348741&r1=348740&r2=348741&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Dec 10 00:19:38 2018
@@ -2861,11 +2861,7 @@ public:
 
   /// Find the failed Boolean condition within a given Boolean
   /// constant expression, and describe it with a string.
-  ///
-  /// \param AllowTopLevelCond Whether to allow the result to be the
-  /// complete top-level condition.
-  std::pair
-  findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond);
+  std::pair findFailedBooleanCondition(Expr *Cond);
 
   /// Emit diagnostics for the diagnose_if attributes on Function, ignoring any
   /// non-ArgDependent DiagnoseIfAttrs.

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=348741&r1=348740&r2=348741&view=diff
==
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Dec 10 00:19:38 2018
@@ -13878,8 +13878,7 @@ Decl *Sema::BuildStaticAssertDeclaration
   Expr *InnerCond = nullptr;
   std::string InnerCondDescription;
   std::tie(InnerCond, InnerCondDescription) =
-findFailedBooleanCondition(Converted.get(),
-   /*AllowTopLevelCond=*/false);
+findFailedBooleanCondition(Converted.get());
   if (InnerCond) {
 Diag(StaticAssertLoc, diag::err_static_assert_requirement_failed)
   << InnerCondDescription << !AssertMessage

Modified: cfe/trunk/lib/Sema/SemaTemplate.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplate.cpp?rev=348741&r1=348740&r2=348741&view=diff
==
--- cfe/trunk/lib/Sema/SemaTemplate.cpp (original)
+++ cfe/trunk/lib/Sema/SemaTemplate.cpp Mon Dec 10 00:19:38 2018
@@ -3052,30 +3052,42 @@ static Expr *lookThroughRangesV3Conditio
   return Cond;
 }
 
-// Print a diagnostic for the failing static_assert expression. Defaults to
-// pretty-printing the expression.
-static void prettyPrintFailedBooleanCondition(llvm::raw_string_ostream &OS,
-  const Expr *FailedCond,
-  const PrintingPolicy &Policy) {
-  const auto *DR = dyn_cast(FailedCond);
-  if (DR && DR->getQualifier()) {
-// If this is a qualified name, expand the template arguments in nested
-// qualifiers.
-DR->getQualifier()->print(OS, Policy, true);
-// Then print the decl itself.
-const ValueDecl *VD = DR->getDecl();
-OS << VD->getName();
-if (const auto *IV = dyn_cast(VD)) {
-  // This is a template variable, print the expanded template arguments.
-  printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+namespace {
+
+// A PrinterHelper that prints more helpful diagnostics for some 
sub-expressions
+// within failing boolean expression, such as substituting template parameters
+// for actual types.
+class FailedBooleanConditionPrinterHelper : public PrinterHelper {
+public:
+  explicit FailedBooleanConditionPrinterHelper(const PrintingPolicy &Policy)
+  : Policy(Policy) {}
+
+  bool handledStmt(Stmt *E, raw_ostream &OS) override {
+const auto *DR = dyn_cast(E);
+if (DR && DR->getQualifier()) {
+  // If this is a qualified name, expand the template arguments in nested
+  // qualifiers.
+  DR->getQualifier()->print(OS, Policy, true);
+  // Then print the decl itself.
+  const ValueDecl *VD = DR->getDecl();
+  OS << VD->getName();
+  if (const auto *IV = dyn_cast(VD)) {
+// This is a template variable, print the expanded template arguments.
+printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+  }
+  return true;
 }
-return;
+return false;
   }
-  FailedCond->printPretty(OS, nullptr, Policy);
-}
+
+private:
+  const PrintingPolicy &Policy;
+};
+
+} // end anonymous namespace
 
 std::pair
-Sem

[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.

2018-12-10 Thread Clement Courbet via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348741: [Sema] Further improvements to to static_assert 
diagnostics. (authored by courbet, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55270/new/

https://reviews.llvm.org/D55270

Files:
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaDeclCXX.cpp
  cfe/trunk/lib/Sema/SemaTemplate.cpp
  cfe/trunk/test/PCH/cxx-static_assert.cpp
  cfe/trunk/test/Sema/static-assert.c
  cfe/trunk/test/SemaCXX/static-assert-cxx17.cpp
  cfe/trunk/test/SemaCXX/static-assert.cpp

Index: cfe/trunk/lib/Sema/SemaTemplate.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplate.cpp
+++ cfe/trunk/lib/Sema/SemaTemplate.cpp
@@ -3052,30 +3052,42 @@
   return Cond;
 }
 
-// Print a diagnostic for the failing static_assert expression. Defaults to
-// pretty-printing the expression.
-static void prettyPrintFailedBooleanCondition(llvm::raw_string_ostream &OS,
-  const Expr *FailedCond,
-  const PrintingPolicy &Policy) {
-  const auto *DR = dyn_cast(FailedCond);
-  if (DR && DR->getQualifier()) {
-// If this is a qualified name, expand the template arguments in nested
-// qualifiers.
-DR->getQualifier()->print(OS, Policy, true);
-// Then print the decl itself.
-const ValueDecl *VD = DR->getDecl();
-OS << VD->getName();
-if (const auto *IV = dyn_cast(VD)) {
-  // This is a template variable, print the expanded template arguments.
-  printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+namespace {
+
+// A PrinterHelper that prints more helpful diagnostics for some sub-expressions
+// within failing boolean expression, such as substituting template parameters
+// for actual types.
+class FailedBooleanConditionPrinterHelper : public PrinterHelper {
+public:
+  explicit FailedBooleanConditionPrinterHelper(const PrintingPolicy &Policy)
+  : Policy(Policy) {}
+
+  bool handledStmt(Stmt *E, raw_ostream &OS) override {
+const auto *DR = dyn_cast(E);
+if (DR && DR->getQualifier()) {
+  // If this is a qualified name, expand the template arguments in nested
+  // qualifiers.
+  DR->getQualifier()->print(OS, Policy, true);
+  // Then print the decl itself.
+  const ValueDecl *VD = DR->getDecl();
+  OS << VD->getName();
+  if (const auto *IV = dyn_cast(VD)) {
+// This is a template variable, print the expanded template arguments.
+printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+  }
+  return true;
 }
-return;
+return false;
   }
-  FailedCond->printPretty(OS, nullptr, Policy);
-}
+
+private:
+  const PrintingPolicy &Policy;
+};
+
+} // end anonymous namespace
 
 std::pair
-Sema::findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond) {
+Sema::findFailedBooleanCondition(Expr *Cond) {
   Cond = lookThroughRangesV3Condition(PP, Cond);
 
   // Separate out all of the terms in a conjunction.
@@ -3087,11 +3099,6 @@
   for (Expr *Term : Terms) {
 Expr *TermAsWritten = Term->IgnoreParenImpCasts();
 
-// Literals are uninteresting.
-if (isa(TermAsWritten) ||
-isa(TermAsWritten))
-  continue;
-
 // The initialization of the parameter from the argument is
 // a constant-evaluated context.
 EnterExpressionEvaluationContext ConstantEvaluated(
@@ -3104,18 +3111,18 @@
   break;
 }
   }
-
-  if (!FailedCond) {
-if (!AllowTopLevelCond)
-  return { nullptr, "" };
-
+  if (!FailedCond)
 FailedCond = Cond->IgnoreParenImpCasts();
-  }
+
+  // Literals are uninteresting.
+  if (isa(FailedCond) || isa(FailedCond))
+return {nullptr, ""};
 
   std::string Description;
   {
 llvm::raw_string_ostream Out(Description);
-prettyPrintFailedBooleanCondition(Out, FailedCond, getPrintingPolicy());
+FailedBooleanConditionPrinterHelper Helper(getPrintingPolicy());
+FailedCond->printPretty(Out, &Helper, getPrintingPolicy());
   }
   return { FailedCond, Description };
 }
@@ -3199,9 +3206,7 @@
 Expr *FailedCond;
 std::string FailedDescription;
 std::tie(FailedCond, FailedDescription) =
-  findFailedBooleanCondition(
-TemplateArgs[0].getSourceExpression(),
-/*AllowTopLevelCond=*/true);
+  findFailedBooleanCondition(TemplateArgs[0].getSourceExpression());
 
 // Remove the old SFINAE diagnostic.
 PartialDiagnosticAt OldDiag =
@@ -9649,7 +9654,7 @@
 Expr *FailedCond;
 std::string FailedDescription;
 std::tie(FailedCond, FailedDescription) =
-  findFailedBooleanCondition(Cond, /*AllowTopLevelCond=*/true);
+  findFailedBooleanCondition(Cond);
 
 Diag(FailedCond->getExprLo

[PATCH] D55482: [clang-tidy] Improve google-objc-function-naming diagnostics 📙

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added inline comments.



Comment at: test/clang-tidy/google-objc-function-naming.m:8
+// must be in Pascal case as required by Google Objective-C style guide
 // CHECK-FIXES: static bool Ispositive(int a) { return a > 0; }
 

I realize there are words that begin with 'is...' but you could detect if the 
function returned a boolean and started with "is,has,does" and could this 
extrapolate to  IsPositive()  HasSomething(), DoesHaveSomething(),  this might 
generate a better fixit candidate? (just a suggestion feel free to ignore)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55482/new/

https://reviews.llvm.org/D55482



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

r348742 - Revert r348741 "[Sema] Further improvements to to static_assert diagnostics."

2018-12-10 Thread Clement Courbet via cfe-commits

Author: courbet
Date: Mon Dec 10 00:53:17 2018
New Revision: 348742

URL: http://llvm.org/viewvc/llvm-project?rev=348742&view=rev
Log:
Revert r348741 "[Sema] Further improvements to to static_assert diagnostics."

Seems to break build bots.

Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Sema/SemaTemplate.cpp
cfe/trunk/test/PCH/cxx-static_assert.cpp
cfe/trunk/test/Sema/static-assert.c
cfe/trunk/test/SemaCXX/static-assert-cxx17.cpp
cfe/trunk/test/SemaCXX/static-assert.cpp

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=348742&r1=348741&r2=348742&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Dec 10 00:53:17 2018
@@ -2861,7 +2861,11 @@ public:
 
   /// Find the failed Boolean condition within a given Boolean
   /// constant expression, and describe it with a string.
-  std::pair findFailedBooleanCondition(Expr *Cond);
+  ///
+  /// \param AllowTopLevelCond Whether to allow the result to be the
+  /// complete top-level condition.
+  std::pair
+  findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond);
 
   /// Emit diagnostics for the diagnose_if attributes on Function, ignoring any
   /// non-ArgDependent DiagnoseIfAttrs.

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=348742&r1=348741&r2=348742&view=diff
==
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Dec 10 00:53:17 2018
@@ -13878,7 +13878,8 @@ Decl *Sema::BuildStaticAssertDeclaration
   Expr *InnerCond = nullptr;
   std::string InnerCondDescription;
   std::tie(InnerCond, InnerCondDescription) =
-findFailedBooleanCondition(Converted.get());
+findFailedBooleanCondition(Converted.get(),
+   /*AllowTopLevelCond=*/false);
   if (InnerCond) {
 Diag(StaticAssertLoc, diag::err_static_assert_requirement_failed)
   << InnerCondDescription << !AssertMessage

Modified: cfe/trunk/lib/Sema/SemaTemplate.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplate.cpp?rev=348742&r1=348741&r2=348742&view=diff
==
--- cfe/trunk/lib/Sema/SemaTemplate.cpp (original)
+++ cfe/trunk/lib/Sema/SemaTemplate.cpp Mon Dec 10 00:53:17 2018
@@ -3052,42 +3052,30 @@ static Expr *lookThroughRangesV3Conditio
   return Cond;
 }
 
-namespace {
-
-// A PrinterHelper that prints more helpful diagnostics for some 
sub-expressions
-// within failing boolean expression, such as substituting template parameters
-// for actual types.
-class FailedBooleanConditionPrinterHelper : public PrinterHelper {
-public:
-  explicit FailedBooleanConditionPrinterHelper(const PrintingPolicy &Policy)
-  : Policy(Policy) {}
-
-  bool handledStmt(Stmt *E, raw_ostream &OS) override {
-const auto *DR = dyn_cast(E);
-if (DR && DR->getQualifier()) {
-  // If this is a qualified name, expand the template arguments in nested
-  // qualifiers.
-  DR->getQualifier()->print(OS, Policy, true);
-  // Then print the decl itself.
-  const ValueDecl *VD = DR->getDecl();
-  OS << VD->getName();
-  if (const auto *IV = dyn_cast(VD)) {
-// This is a template variable, print the expanded template arguments.
-printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
-  }
-  return true;
+// Print a diagnostic for the failing static_assert expression. Defaults to
+// pretty-printing the expression.
+static void prettyPrintFailedBooleanCondition(llvm::raw_string_ostream &OS,
+  const Expr *FailedCond,
+  const PrintingPolicy &Policy) {
+  const auto *DR = dyn_cast(FailedCond);
+  if (DR && DR->getQualifier()) {
+// If this is a qualified name, expand the template arguments in nested
+// qualifiers.
+DR->getQualifier()->print(OS, Policy, true);
+// Then print the decl itself.
+const ValueDecl *VD = DR->getDecl();
+OS << VD->getName();
+if (const auto *IV = dyn_cast(VD)) {
+  // This is a template variable, print the expanded template arguments.
+  printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
 }
-return false;
+return;
   }
-
-private:
-  const PrintingPolicy &Policy;
-};
-
-} // end anonymous namespace
+  FailedCond->printPretty(OS, nullptr, Policy);
+}
 
 std::pair
-Sema::findFailedBooleanCondition(Expr *Cond) {
+Sema::findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond) {
   Cond = lookThroughRangesV3Condition(PP, Cond);

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

Just a few minor remarks and a possible workaround for testing `CHECK-FIXES: 
[[nodiscard]]`.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:43
+  // function like _Foo()
+  if (ignore){
+ return doesFunctionNameStartWithUnderScore(&Node);

If think that you should run clang-format over your patch.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:118
+  // 2. a const member function which return a variable which is ignored
+  // performs some externation io operation and the return value could be 
ignore
+

Please fix typos/grammar, externation -> external, io -> I/O, ignore -> ignored.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Users can use :option:`ReplacementString` to specify a macro to use instead of
+``[[nodiscard]]``. This is useful when maintaining source code that needs to

I would avoid repeating the option name in its description and state clearly 
that `[[nodiscard]]` is the default.

Cf. other clang-tidy checks with options: 
http://clang.llvm.org/extra/clang-tidy/checks/list.html, e.g. 
http://clang.llvm.org/extra/clang-tidy/checks/readability-function-size.html#options.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:39
+``[[nodiscard]]``. This is useful when maintaining source code that needs to
+also compile with a non c++17 conforming compiler.
+

I'd suggest "needs to compile with a pre-C++17 compiler."



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:61
+
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()

"turn off the adding"? I'm not a native English speaker, but "turn off addition 
of" would sound better.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:61
+
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()

curdeius wrote:
> "turn off the adding"? I'm not a native English speaker, but "turn off 
> addition of" would sound better.
Same as for the other option, I wouldn't repeat the option name and give the 
default value explicitly.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

curdeius wrote:
> Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
It might have been discussed already, but is it a common convention to mark 
internal functions with a leading underscore?
If that comes from system headers, AFAIK clang-tidy already is able not to emit 
warnings from them.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:63
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;

I think you can use a regex as a workaround, i.e. using `{{\[\[nodiscard\]\]}}`.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:66
+// CHECK-MESSAGES-NOT: warning:
+// _CHECK-FIXES: [[nodiscard]] bool f11() const;
+

So this line should become: `// CHECK-FIXES: {{\[\[nodiscard\]\] bool f11() 
const;`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

In D55433#1323779 , @lebedev.ri wrote:

> In D55433#1323757 , @MyDeveloperDay 
> wrote:
>
> > a lot of projects aren't setup for c++17 yet which is needed for 
> > [[nodiscard]] to be allowed,
>
>
> You can use `[[clang::warn_unused_result]]` for this evaluation, that does 
> not require C++17.
>
> > But the clang-tidy -fix doesn't break the build, the patch is large but 
> > nothing broke. (at compile time at least!)
> > 
> >   {F7661182} 
>
> Uhm, so there wasn't a single true-positive in protobuf?


To elaborate, what i'm asking is:

- Is that project -Werror clean without those changes?
- if yes, after all those changes, does the -Werror build break?
  - If it does break, how many of those issues are actual bugs (ignored return 
when it should not be ignored), and how many are noise.
  - If not, then i guess all these were "false-positives"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

2018-12-10 Thread Gabor Marton via Phabricator via cfe-commits

martong added a comment.

@rsmith Please raise any objections until Dec 14 (or if this deadline is too 
strict). I'd like to commit this next week latest so it can get in still this 
year.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53655/new/

https://reviews.llvm.org/D53655



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay updated this revision to Diff 177474.
MyDeveloperDay marked 11 inline comments as done.
MyDeveloperDay added a comment.

- Addressing review comments and concerns
- Removed internal function logic and option, not really the role of this 
checker
- Fixed grammatical error in documentation
- enabled fix-it check in unit tests for  already present [[nodiscard]] by 
using escaping which allowed use of [[]] in CHECK-FIXES
- minor clang format issue


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warnin

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:43
+  // function like _Foo()
+  if (ignore){
+ return doesFunctionNameStartWithUnderScore(&Node);

curdeius wrote:
> If think that you should run clang-format over your patch.
Sorry my bad, I do have clang-format to run on save which should pick up your 
style, but I forgot you guys don't like braces on small ifs...



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

curdeius wrote:
> curdeius wrote:
> > Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
> It might have been discussed already, but is it a common convention to mark 
> internal functions with a leading underscore?
> If that comes from system headers, AFAIK clang-tidy already is able not to 
> emit warnings from them.
If you feel this is an unnecessary option, I'm happy to remove it, it might 
simplify things.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:62
+Users can use :option:`IgnoreInternalFunctions` to turn off the adding of
+``[nodiscard]]`` to functions starting with _ e.g. _Foo()
+

MyDeveloperDay wrote:
> curdeius wrote:
> > curdeius wrote:
> > > Missing leading `[` in `[nodiscard]]`, should be `[[nodiscard]]`
> > It might have been discussed already, but is it a common convention to mark 
> > internal functions with a leading underscore?
> > If that comes from system headers, AFAIK clang-tidy already is able not to 
> > emit warnings from them.
> If you feel this is an unnecessary option, I'm happy to remove it, it might 
> simplify things.
on reflection as both @curdeius  and other reviews expressed concern of 
"isInternalFunction" motivation, I decided to remove this as an option, I think 
I was letting my usecase get mixed up with the design


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added a comment.

In D55433#1325117 , @lebedev.ri wrote:

> In D55433#1323779 , @lebedev.ri 
> wrote:
>
> > In D55433#1323757 , 
> > @MyDeveloperDay wrote:
> >
> > > a lot of projects aren't setup for c++17 yet which is needed for 
> > > [[nodiscard]] to be allowed,
> >
> >
> > You can use `[[clang::warn_unused_result]]` for this evaluation, that does 
> > not require C++17.
> >
> > > But the clang-tidy -fix doesn't break the build, the patch is large but 
> > > nothing broke. (at compile time at least!)
> > > 
> > >   {F7661182} 
> >
> > Uhm, so there wasn't a single true-positive in protobuf?
>
>
> To elaborate, what i'm asking is:
>
> - Is that project -Werror clean without those changes?
> - if yes, after all those changes, does the -Werror build break?
>   - If it does break, how many of those issues are actual bugs (ignored 
> return when it should not be ignored), and how many are noise.
>   - If not, then i guess all these were "false-positives"

I totally get where you are coming from, and I want to answer correctly...

1. protobuf was clean to begin with when compiling with -Werror
2. after applying nodiscard fix-it protobuf would not build cleanly with 
-Werror but will build cleanly without it
3. those warnigns ARE related to the introduction of the [[nodiscard]]

  F7673134: image.png 

taking the first one as an example, I'm trying to determine if

  int ExtensionSet::NumExtensions() const {
int result = 0;
ForEach([&result](int /* number */, const Extension& ext) {
  ^^^
  if (!ext.is_cleared) {
++result;
  }
});
return result;
  }

having the checker added nodiscard..

  template 
  [[nodiscard]] KeyValueFunctor ForEach(KeyValueFunctor func) const {
if (PROTOBUF_PREDICT_FALSE(is_large())) {
  return ForEach(map_.large->begin(), map_.large->end(), std::move(func));
}
return ForEach(flat_begin(), flat_end(), std::move(func));
  }

Is a false positive, or is Visual C++ somehow thinking the return value is not 
used when its a lambda? but I get similar issues when compiling with the Intel 
compiler, so I think its more likely that the return value from ForEach isn't 
actually used, and the implementation is just allowing ForEach()'s to be 
chained together

F7673175: image.png 

All of these new warnings introduced are related to the use of a lambda with 
the exception of

  // We don't care what this returns since we'll find out below anyway.
  pool_->TryFindFileInFallbackDatabase(proto.dependency(i));

Which you could say that it should be written as

  [[maybe_unused]] auto rt= 
pool_->TryFindFileInFallbackDatabase(proto.dependency(i));

But the effect of applying the fix-it, doesn't itself break the build, its the 
effect of [[nodiscard]] having been added. (perhaps incorrectly)

However the build is not broken when NOT using warnings as errors, and so it 
depends on the "ethos" of clang-tidy, if the goal is for clang-tidy to remain 
error/warning free after applying -fix then i can see the fix-it generating 
changes that causes warnings isn't correct and that should be considered a 
false positive and I should consider not emitting the [[nodiscard]] when the 
argument is perhaps a lambda.

I guess in the past I've used clang-tidy to also help me generate new warnings 
so I can fix that code, but that may not be the goal of its usage.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433

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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

2018-12-10 Thread Mikael Nilsson via Phabricator via cfe-commits

mikael updated this revision to Diff 177477.
mikael marked an inline comment as not done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54862/new/

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/CanonicalType.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDumper.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/AST/Type.cpp
  lib/AST/TypePrinter.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGValue.h
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  test/CodeGenOpenCLCXX/template-address-spaces.cl
  test/SemaOpenCLCXX/address-space-templates.cl
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8370,7 +8370,7 @@
   const Decl *D = cxcursor::getCursorDecl(C);
   const CXXMethodDecl *Method =
   D ? dyn_cast_or_null(D->getAsFunction()) : nullptr;
-  return (Method && (Method->getTypeQualifiers() & Qualifiers::Const)) ? 1 : 0;
+  return (Method && Method->getTypeQualifiers().hasConst()) ? 1 : 0;
 }
 
 unsigned clang_CXXMethod_isDefaulted(CXCursor C) {
Index: test/SemaOpenCLCXX/address-space-templates.cl
===
--- test/SemaOpenCLCXX/address-space-templates.cl
+++ test/SemaOpenCLCXX/address-space-templates.cl
@@ -4,7 +4,9 @@
 struct S {
   T a;// expected-error{{field may not be qualified with an address space}}
   T f1(); // expected-error{{function type may not be qualified with an address space}}
-  void f2(T); // expected-error{{parameter may not be qualified with an address space}}
+  // FIXME: Should only get the error message once.
+  void f2(T); // expected-error{{parameter may not be qualified with an address space}} expected-error{{parameter may not be qualified with an address space}}
+
 };
 
 template 
Index: test/CodeGenOpenCLCXX/template-address-spaces.cl
===
--- test/CodeGenOpenCLCXX/template-address-spaces.cl
+++ test/CodeGenOpenCLCXX/template-address-spaces.cl
@@ -9,13 +9,16 @@
 template
 T S::foo() { return a;}
 
-//CHECK: %struct.S = type { i32 }
-//CHECK: %struct.S.0 = type { i32 addrspace(4)* }
-//CHECK: %struct.S.1 = type { i32 addrspace(1)* }
+// CHECK: %struct.S = type { i32 }
+// CHECK: %struct.S.0 = type { i32 addrspace(4)* }
+// CHECK: %struct.S.1 = type { i32 addrspace(1)* }
 
-//CHECK: i32 @_ZN1SIiE3fooEv(%struct.S* %this)
-//CHECK: i32 addrspace(4)* @_ZN1SIPU3AS4iE3fooEv(%struct.S.0* %this)
-//CHECK: i32 addrspace(1)* @_ZN1SIPU3AS1iE3fooEv(%struct.S.1* %this)
+// CHECK:  %0 = addrspacecast %struct.S* %sint to %struct.S addrspace(4)*
+// CHECK:  %call = call i32 @_ZNU3AS41SIiE3fooEv(%struct.S addrspace(4)* %0) #1
+// CHECK:  %1 = addrspacecast %struct.S.0* %sintptr to %struct.S.0 addrspace(4)*
+// CHECK:  %call1 = call i32 addrspace(4)* @_ZNU3AS41SIPU3AS4iE3fooEv(%struct.S.0 addrspace(4)* %1) #1
+// CHECK:  %2 = addrspacecast %struct.S.1* %sintptrgl to %struct.S.1 addrspace(4)*
+// CHECK:  %call2 = call i32 addrspace(1)* @_ZNU3AS41SIPU3AS1iE3fooEv(%struct.S.1 addrspace(4)* %2) #1
 
 void bar(){
   S sint;
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,154 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  // FIXME: Does not work yet.
+  // C(C &&c) { v = c.v; }
+  C(const C &c) { v = c.v; }
+  C &operator=(const C &c) {
+v = c.v;
+return *this;
+  }
+  // FIXME: Does not work yet.
+  //C &operator=(C&& c) & {
+  //  v = c.v;
+  //  return *this;
+  //}
+
+  int get() { return v; }
+
+  int outside();
+};
+
+int C::outside() {
+  return v;
+}
+
+extern C&& foo();
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  int i2 = c.outside();
+  C c1(c);
+  C c2;
+  c2

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

2018-12-10 Thread Mikael Nilsson via Phabricator via cfe-commits

mikael added a comment.

I rebased on Friday, and noticed that I broke two tests:

Failing Tests (2):

  Clang :: CodeGenOpenCLCXX/template-address-spaces.cl
  Clang :: SemaOpenCLCXX/address-space-templates.cl

This upload contains a few extra fixes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54862/new/

https://reviews.llvm.org/D54862



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

2018-12-10 Thread Mikael Nilsson via Phabricator via cfe-commits

mikael marked 4 inline comments as done.
mikael added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:3196
+
+QualType AdjustedQT = QualType(AdjustedType, 0);
+LangAS AS = Old->getType().getAddressSpace();

When merging the class function and the file context function the address space 
was lost here.



Comment at: lib/Sema/SemaType.cpp:4831
+if (state.getSema().getLangOpts().OpenCLCPlusPlus &&
+((DC && DC->isRecord()) ||
+ state.getDeclarator().getContext() ==

The first issue was that if a function is declared outside of a class this code 
did not deduce it. Now it does.



Comment at: lib/Sema/TreeTransform.h:4277
   //   cv-qualifiers are ignored.
-  if (T->isFunctionType())
+  // OpenCL: The address space should not be ignored.
+  if (T->isFunctionType()) {

Another place where the address space was removed.



Comment at: test/SemaOpenCLCXX/address-space-templates.cl:7
   T f1(); // expected-error{{function type may not be qualified with an 
address space}}
-  void f2(T); // expected-error{{parameter may not be qualified with an 
address space}}
+  // FIXME: Should only get the error message once.
+  void f2(T); // expected-error{{parameter may not be qualified with an 
address space}} expected-error{{parameter may not be qualified with an address 
space}}

This was the remaining issue that I have not solved yet. It looked like this 
issue was not so trivial so I think it makes sense to postpone it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54862/new/

https://reviews.llvm.org/D54862



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

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

2018-12-10 Thread Endre Fülöp via Phabricator via cfe-commits

gamesh411 added a comment.

In my opinion, after migrating relevant test cases from D33826 
, this is ready.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D48866/new/

https://reviews.llvm.org/D48866



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

2018-12-10 Thread Reuben Thomas via Phabricator via cfe-commits

reuk added a subscriber: klimek.
reuk added a comment.

Would someone review this please? I'm not sure who to add for review (sorry), 
maybe one of the following?
@klimek @Typz @krasimir


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55170/new/

https://reviews.llvm.org/D55170



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

[PATCH] D55428: [Docs] Expand -fstack-protector and -fstack-protector-all info

2018-12-10 Thread Thomas Preud'homme via Phabricator via cfe-commits

thopre added inline comments.



Comment at: include/clang/Driver/Options.td:1634-1636
+  HelpText<"Uses a stronger heuristic to apply stack protectors to functions "
+   "that include arrays of any size (and any type), "
+   "as well as any calls to alloca or the taking of an address from a 
local variable">;

I'm not sure what's the policy for related options but I feel the description 
should stand on its own. I'd therefor start by:

"Enable stack protectors for some functions potentially vulnerable to stack 
smashing. Compared to -fstack-protector, this uses a stronger heuristic ()"

If the policy is to avoid such repeatition then please ignore this comment.



Comment at: include/clang/Driver/Options.td:1638
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
-  HelpText<"Enable stack protectors for functions potentially vulnerable to 
stack smashing">;
+  HelpText<"Enable stack protectors for some functions potentially vulnerable 
to stack smashing. "
+   "Namely those containing a char (or 8bit integer) array or constant 
sized calls to "

Not a native english speaker but I feel that "potentially" is redundant given 
you said it enables stack protector for *some* functions. Perhaps rewrite it 
along the lines of:

"Enable stack protectors for some of the functions vulnerable to stack smashing 
based on simple heuristic"

with a better word than "simple". This conveys both that not all functions are 
protected and suggests that a better heuristic is possible. You can then easily 
refer the reader to -fstack-protector-strong and -fstack-protector-all in a 
following sentence.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55428/new/

https://reviews.llvm.org/D55428



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

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

2018-12-10 Thread Mikael Holmén via Phabricator via cfe-commits

uabelho added a comment.

Hi,
A late question about this change. I notice that this change sometimes gives me 
additional DIFiles in the clang output compared to before.
E.g. if I have a file

/tmp/bar/foo.c

containing just

void foo() {
 }

and I stand in /tmp/ and do

clang -emit-llvm -S -g /tmp/bar/foo.c -o -

then I get two DIFiles in the output:

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang 
version 8.0.0 (trunk 348738) (llvm/trunk 348737)", isOptimized: false, 
runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
 !1 = !DIFile(filename: "/tmp/bar/foo.c", directory: "/tmp")
 [...]
 !8 = !DIFile(filename: "bar/foo.c", directory: "/tmp")

where !1 is only used in !DICompileUnit and !8 is used everywhere else. Before 
this change the only DIFile I got was

!1 = !DIFile(filename: "/tmp/bar/foo.c", directory: "/tmp")

so in the clang output we actually got more tuff with the change than before.

Also, for my out-of-tree target the two DIFiles later caused confusion for gdb 
since there then was a mismatch between the compilation unit name
in the debug line section and in the debug info section. Due to that mismatch 
gdb dropped the prologue_end so in cases where foo had a more interesting body
I would stop at the wrong place if I set a breakpoint with
 break foo

I haven't seen the problem with a breakpoint ending up at the wrong place when 
compiling for X86 though and I'm not sure
if it's because I haven't tried hard enough or if it's something special with 
my backend.

Anyway, is the above as expected or should the filename in !1 be shortened as 
well?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55085/new/

https://reviews.llvm.org/D55085



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

r348752 - [OpenCL][CodeGen] Fix replacing memcpy with addrspacecast

2018-12-10 Thread Andrew Savonichev via cfe-commits

Author: asavonic
Date: Mon Dec 10 04:03:00 2018
New Revision: 348752

URL: http://llvm.org/viewvc/llvm-project?rev=348752&view=rev
Log:
[OpenCL][CodeGen] Fix replacing memcpy with addrspacecast

Summary:
If a function argument is byval and RV is located in default or alloca address 
space
an optimization of creating addrspacecast instead of memcpy is performed. That 
is
not correct for OpenCL, where that can lead to a situation of address space 
casting
from __private * to __global *. See an example below:

```
typedef struct {
  int x;
} MyStruct;

void foo(MyStruct val) {}

kernel void KernelOneMember(__global MyStruct* x) {
  foo (*x);
}
```

for this code clang generated following IR:
...
%0 = load %struct.MyStruct addrspace(1)*, %struct.MyStruct addrspace(1)**
%x.addr, align 4
%1 = addrspacecast %struct.MyStruct addrspace(1)* %0 to %struct.MyStruct*
...

So the optimization was disallowed for OpenCL if RV is located in an address 
space
different than that of the argument (0).


Reviewers: yaxunl, Anastasia

Reviewed By: Anastasia

Subscribers: cfe-commits, asavonic

Differential Revision: https://reviews.llvm.org/D54947

Modified:
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/test/CodeGenOpenCL/addr-space-struct-arg.cl

Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=348752&r1=348751&r2=348752&view=diff
==
--- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCall.cpp Mon Dec 10 04:03:00 2018
@@ -3958,15 +3958,28 @@ RValue CodeGenFunction::EmitCall(const C
 } else if (I->hasLValue()) {
   auto LV = I->getKnownLValue();
   auto AS = LV.getAddressSpace();
+
   if ((!ArgInfo.getIndirectByVal() &&
(LV.getAlignment() >=
-getContext().getTypeAlignInChars(I->Ty))) ||
-  (ArgInfo.getIndirectByVal() &&
-   ((AS != LangAS::Default && AS != LangAS::opencl_private &&
- AS != CGM.getASTAllocaAddressSpace() {
+getContext().getTypeAlignInChars(I->Ty {
+NeedCopy = true;
+  }
+  if (!getLangOpts().OpenCL) {
+if ((ArgInfo.getIndirectByVal() &&
+(AS != LangAS::Default &&
+ AS != CGM.getASTAllocaAddressSpace( {
+  NeedCopy = true;
+}
+  }
+  // For OpenCL even if RV is located in default or alloca address 
space
+  // we don't want to perform address space cast for it.
+  else if ((ArgInfo.getIndirectByVal() &&
+Addr.getType()->getAddressSpace() != IRFuncTy->
+  getParamType(FirstIRArg)->getPointerAddressSpace())) {
 NeedCopy = true;
   }
 }
+
 if (NeedCopy) {
   // Create an aligned temporary, and copy to it.
   Address AI = CreateMemTempWithoutCast(

Modified: cfe/trunk/test/CodeGenOpenCL/addr-space-struct-arg.cl
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenOpenCL/addr-space-struct-arg.cl?rev=348752&r1=348751&r2=348752&view=diff
==
--- cfe/trunk/test/CodeGenOpenCL/addr-space-struct-arg.cl (original)
+++ cfe/trunk/test/CodeGenOpenCL/addr-space-struct-arg.cl Mon Dec 10 04:03:00 
2018
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header 
-ffake-address-space-map -triple i686-pc-darwin | FileCheck -enable-var-scope 
-check-prefixes=COM,X86 %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -triple 
amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 
-finclude-default-header -triple amdgcn | FileCheck -enable-var-scope 
-check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -ffake-address-space-map -triple 
i686-pc-darwin | FileCheck -enable-var-scope -check-prefixes=COM,X86 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -triple amdgcn | FileCheck 
-enable-var-scope -check-prefixes=COM,AMDGCN %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -triple amdgcn | 
FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL1.2 -O0 -triple 
spir-unknown-unknown-unknown | FileCheck -enable-var-scope -check-prefixes=SPIR 
%s
+
+typedef int int2 __attribute__((ext_vector_type(2)));
 
 typedef struct {
   int cells[9];
@@ -130,6 +133,12 @@ kernel void KernelOneMember(struct Struc
   FuncOneMember(u);
 }
 
+// SPIR: call void @llvm.memcpy.p0i8.p1i8.i32
+// SPIR-NOT: addrspacecast
+kernel void KernelOneMemberSpir(global struct StructOneMember* u) {
+  FuncOneMember(*u);
+}
+
 // AMDGCN-LABEL: define amdgpu_kernel void @KernelLargeOneMember(
 // AMDGCN:  %[[U:.*]] = alloca %struct.LargeStructOneMember, align 8,

[PATCH] D54947: [OpenCL][CodeGen] Fix replacing memcpy with addrspacecast

2018-12-10 Thread Andrew Savonichev via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348752: [OpenCL][CodeGen] Fix replacing memcpy with 
addrspacecast (authored by asavonic, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54947/new/

https://reviews.llvm.org/D54947

Files:
  lib/CodeGen/CGCall.cpp
  test/CodeGenOpenCL/addr-space-struct-arg.cl


Index: test/CodeGenOpenCL/addr-space-struct-arg.cl
===
--- test/CodeGenOpenCL/addr-space-struct-arg.cl
+++ test/CodeGenOpenCL/addr-space-struct-arg.cl
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header 
-ffake-address-space-map -triple i686-pc-darwin | FileCheck -enable-var-scope 
-check-prefixes=COM,X86 %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -triple 
amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 
-finclude-default-header -triple amdgcn | FileCheck -enable-var-scope 
-check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -ffake-address-space-map -triple 
i686-pc-darwin | FileCheck -enable-var-scope -check-prefixes=COM,X86 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -triple amdgcn | FileCheck 
-enable-var-scope -check-prefixes=COM,AMDGCN %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -triple amdgcn | 
FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL1.2 -O0 -triple 
spir-unknown-unknown-unknown | FileCheck -enable-var-scope -check-prefixes=SPIR 
%s
+
+typedef int int2 __attribute__((ext_vector_type(2)));
 
 typedef struct {
   int cells[9];
@@ -130,6 +133,12 @@
   FuncOneMember(u);
 }
 
+// SPIR: call void @llvm.memcpy.p0i8.p1i8.i32
+// SPIR-NOT: addrspacecast
+kernel void KernelOneMemberSpir(global struct StructOneMember* u) {
+  FuncOneMember(*u);
+}
+
 // AMDGCN-LABEL: define amdgpu_kernel void @KernelLargeOneMember(
 // AMDGCN:  %[[U:.*]] = alloca %struct.LargeStructOneMember, align 8, 
addrspace(5)
 // AMDGCN:  store %struct.LargeStructOneMember %u.coerce, 
%struct.LargeStructOneMember addrspace(5)* %[[U]], align 8
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -3958,15 +3958,28 @@
 } else if (I->hasLValue()) {
   auto LV = I->getKnownLValue();
   auto AS = LV.getAddressSpace();
+
   if ((!ArgInfo.getIndirectByVal() &&
(LV.getAlignment() >=
-getContext().getTypeAlignInChars(I->Ty))) ||
-  (ArgInfo.getIndirectByVal() &&
-   ((AS != LangAS::Default && AS != LangAS::opencl_private &&
- AS != CGM.getASTAllocaAddressSpace() {
+getContext().getTypeAlignInChars(I->Ty {
+NeedCopy = true;
+  }
+  if (!getLangOpts().OpenCL) {
+if ((ArgInfo.getIndirectByVal() &&
+(AS != LangAS::Default &&
+ AS != CGM.getASTAllocaAddressSpace( {
+  NeedCopy = true;
+}
+  }
+  // For OpenCL even if RV is located in default or alloca address 
space
+  // we don't want to perform address space cast for it.
+  else if ((ArgInfo.getIndirectByVal() &&
+Addr.getType()->getAddressSpace() != IRFuncTy->
+  getParamType(FirstIRArg)->getPointerAddressSpace())) {
 NeedCopy = true;
   }
 }
+
 if (NeedCopy) {
   // Create an aligned temporary, and copy to it.
   Address AI = CreateMemTempWithoutCast(


Index: test/CodeGenOpenCL/addr-space-struct-arg.cl
===
--- test/CodeGenOpenCL/addr-space-struct-arg.cl
+++ test/CodeGenOpenCL/addr-space-struct-arg.cl
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -ffake-address-space-map -triple i686-pc-darwin | FileCheck -enable-var-scope -check-prefixes=COM,X86 %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -finclude-default-header -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -ffake-address-space-map -triple i686-pc-darwin | FileCheck -enable-var-scope -check-prefixes=COM,X86 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL1.2 -O0 -triple spir-unknown-unknown-unkn

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

2018-12-10 Thread Hyrum Wright via Phabricator via cfe-commits

hwright added a comment.

Reminder: I'll need somebody to submit this for me, since I don't have 
subversion access.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

2018-12-10 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:20-39
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {
+switch (Scale) {
+case DurationScale::Hours:
+  return 0;
+case DurationScale::Minutes:

You should not need this if you change the `enum` instead.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:66-77
+InverseMap[DurationScale::Hours] =
+std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours");
+InverseMap[DurationScale::Minutes] =
+std::make_pair("::absl::ToDoubleMinutes", 
"::absl::ToInt64Minutes");
+InverseMap[DurationScale::Seconds] =
+std::make_pair("::absl::ToDoubleSeconds", 
"::absl::ToInt64Seconds");
+InverseMap[DurationScale::Milliseconds] = std::make_pair(

I was thinking of more like
```
for(std::pair e : std::initializer_list({
   {DurationScale::Hours, 
std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")}.
   }))
  InverseMap[e.first] = e.second;
```



Comment at: clang-tidy/abseil/DurationRewriter.h:22-23
 /// Duration factory and conversion scales
 enum class DurationScale : std::int8_t {
   Hours,
   Minutes,

```
enum class DurationScale : std::int8_t {
  Hours = 0,
  Minutes,
...
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55475: Misc typos fixes in ./lib folder

2018-12-10 Thread Raphael Isemann via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348755: Misc typos fixes in ./lib folder (authored by 
teemperor, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55475/new/

https://reviews.llvm.org/D55475

Files:
  lib/ARCMigrate/FileRemapper.cpp
  lib/AST/ASTContext.cpp
  lib/AST/ASTStructuralEquivalence.cpp
  lib/AST/ExprConstant.cpp
  lib/AST/ODRHash.cpp
  lib/AST/RawCommentList.cpp
  lib/Analysis/CloneDetection.cpp
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CGOpenMPRuntime.h
  lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  lib/CodeGen/CGStmtOpenMP.cpp
  lib/Driver/Driver.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/AMDGPU.cpp
  lib/Driver/ToolChains/Darwin.cpp
  lib/Driver/ToolChains/Hexagon.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Lex/Lexer.cpp
  lib/Lex/PPDirectives.cpp
  lib/Lex/Preprocessor.cpp
  lib/Parse/Parser.cpp
  lib/Rewrite/RewriteRope.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaDeclObjC.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaLookup.cpp
  lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
  lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
  lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  lib/StaticAnalyzer/Core/BugReporter.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  lib/StaticAnalyzer/Core/Store.cpp
  lib/StaticAnalyzer/Core/WorkList.cpp
  lib/StaticAnalyzer/README.txt

Index: lib/Lex/Lexer.cpp
===
--- lib/Lex/Lexer.cpp
+++ lib/Lex/Lexer.cpp
@@ -1015,7 +1015,7 @@
 StringRef Lexer::getImmediateMacroNameForDiagnostics(
 SourceLocation Loc, const SourceManager &SM, const LangOptions &LangOpts) {
   assert(Loc.isMacroID() && "Only reasonable to call this on macros");
-  // Walk past macro argument expanions.
+  // Walk past macro argument expansions.
   while (SM.isMacroArgExpansion(Loc))
 Loc = SM.getImmediateExpansionRange(Loc).getBegin();
 
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -118,7 +118,7 @@
 // the specified module, meaning clang won't build the specified module. This is
 // useful in a number of situations, for instance, when building a library that
 // vends a module map, one might want to avoid hitting intermediate build
-// products containig the the module map or avoid finding the system installed
+// products containimg the the module map or avoid finding the system installed
 // modulemap for that library.
 static bool isForModuleBuilding(Module *M, StringRef CurrentModule,
 StringRef ModuleName) {
@@ -399,7 +399,7 @@
 // If this is the end of the buffer, we have an error.
 if (Tok.is(tok::eof)) {
   // We don't emit errors for unterminated conditionals here,
-  // Lexer::LexEndOfFile can do that propertly.
+  // Lexer::LexEndOfFile can do that properly.
   // Just return and let the caller lex after this #include.
   if (PreambleConditionalStack.isRecording())
 PreambleConditionalStack.SkipInfo.emplace(
Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -931,7 +931,7 @@
   // If we have a non-empty module path, load the named module.
   if (!ModuleImportPath.empty()) {
 // Under the Modules TS, the dot is just part of the module name, and not
-// a real hierarachy separator. Flatten such module names now.
+// a real hierarchy separator. Flatten such module names now.
 //
 // FIXME: Is this the right level to be performing this transformation?
 std::string FlatModuleName;
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -911,7 +911,7 @@
   return true;
 }
 
-/// Determine structural equivalence of two methodss.
+/// Determine structural equivalence of two methods.
 static bool IsStructurallyEq

r348755 - Misc typos fixes in ./lib folder

2018-12-10 Thread Raphael Isemann via cfe-commits

Author: teemperor
Date: Mon Dec 10 04:37:46 2018
New Revision: 348755

URL: http://llvm.org/viewvc/llvm-project?rev=348755&view=rev
Log:
Misc typos fixes in ./lib folder

Summary: Found via `codespell -q 3 -I ../clang-whitelist.txt -L 
uint,importd,crasher,gonna,cant,ue,ons,orign,ned`

Reviewers: teemperor

Reviewed By: teemperor

Subscribers: teemperor, jholewinski, jvesely, nhaehnle, whisperity, jfb, 
cfe-commits

Differential Revision: https://reviews.llvm.org/D55475

Modified:
cfe/trunk/lib/ARCMigrate/FileRemapper.cpp
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/AST/ODRHash.cpp
cfe/trunk/lib/AST/RawCommentList.cpp
cfe/trunk/lib/Analysis/CloneDetection.cpp
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/CodeGen/CGBlocks.cpp
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/CodeGen/CGDebugInfo.h
cfe/trunk/lib/CodeGen/CGObjC.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
cfe/trunk/lib/Driver/Driver.cpp
cfe/trunk/lib/Driver/ToolChain.cpp
cfe/trunk/lib/Driver/ToolChains/AMDGPU.cpp
cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
cfe/trunk/lib/Driver/ToolChains/Hexagon.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/lib/Lex/Lexer.cpp
cfe/trunk/lib/Lex/PPDirectives.cpp
cfe/trunk/lib/Lex/Preprocessor.cpp
cfe/trunk/lib/Parse/Parser.cpp
cfe/trunk/lib/Rewrite/RewriteRope.cpp
cfe/trunk/lib/Sema/Sema.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Sema/SemaDeclObjC.cpp
cfe/trunk/lib/Sema/SemaInit.cpp
cfe/trunk/lib/Sema/SemaLookup.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h

cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp
cfe/trunk/lib/StaticAnalyzer/Core/WorkList.cpp
cfe/trunk/lib/StaticAnalyzer/README.txt

Modified: cfe/trunk/lib/ARCMigrate/FileRemapper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/FileRemapper.cpp?rev=348755&r1=348754&r2=348755&view=diff
==
--- cfe/trunk/lib/ARCMigrate/FileRemapper.cpp (original)
+++ cfe/trunk/lib/ARCMigrate/FileRemapper.cpp Mon Dec 10 04:37:46 2018
@@ -226,7 +226,7 @@ void FileRemapper::remap(const FileEntry
 
 const FileEntry *FileRemapper::getOriginalFile(StringRef filePath) {
   const FileEntry *file = FileMgr->getFile(filePath);
-  // If we are updating a file that overriden an original file,
+  // If we are updating a file that overridden an original file,
   // actually update the original file.
   llvm::DenseMap::iterator
 I = ToFromMappings.find(file);

Modified: cfe/trunk/lib/AST/ASTContext.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=348755&r1=348754&r2=348755&view=diff
==
--- cfe/trunk/lib/AST/ASTContext.cpp (original)
+++ cfe/trunk/lib/AST/ASTContext.cpp Mon Dec 10 04:37:46 2018
@@ -8129,7 +8129,7 @@ void getIntersectionOfProtocols(ASTConte
   // Also add the protocols associated with the LHS interface.
   Context.CollectInheritedProtocols(LHS->getInterface(), LHSProtocolSet);
 
-  // Add all of the protocls for the RHS.
+  // Add all of the protocols for the RHS.
   llvm::SmallPtrSet RHSProtocolSet;
 
   // Start with the protocol qualifiers.

Modified: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp?rev=348755&r1=348754&r2=348755&view=diff
==
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp (original)
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp Mon

[PATCH] D55346: [clang-tidy] check for using declaration qualification

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/QualifiedAliasesCheck.cpp:29
+  // Finds the nested-name-specifier location.
+  const NestedNameSpecifierLoc QualifiedLoc = MatchedDecl->getQualifierLoc();
+  const SourceLocation FrontLoc = QualifiedLoc.getBeginLoc();

Drop the top-level `const` qualifier (here and elsewhere).



Comment at: clang-tidy/abseil/QualifiedAliasesCheck.cpp:34
+  const SourceManager *SM = Result.SourceManager;
+  CharSourceRange FrontRange = CharSourceRange();
+  FrontRange.setBegin(FrontLoc);

No need for the assignment.



Comment at: clang-tidy/abseil/QualifiedAliasesCheck.cpp:39
+
+  // If the using declaration is fully qualified, don't produce a warning.
+  if (Beg.startswith("::"))

This is missing a check for whether the referenced name is within the current 
namespace.



Comment at: clang-tidy/abseil/QualifiedAliasesCheck.cpp:43-44
+
+  diag(FrontLoc, "using declaration is not fully qualified: see "
+  "https://abseil.io/tips/119";);
+}

Do not put HTML links into the diagnostic -- the wording should stand on its 
own. I would probably phrase it "using declaration should use a fully qualified 
name" or something along those lines.



Comment at: clang-tidy/abseil/QualifiedAliasesCheck.h:19
+
+/// FIXME: Write a short description.
+///

This should be fixed.



Comment at: test/clang-tidy/abseil-qualified-aliases.cpp:22-23
+
+using innermost::Color;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: using declaration is not fully 
qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+

I don't think this should warn, per the tip.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55346/new/

https://reviews.llvm.org/D55346



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

[PATCH] D55411: [clang-tidy] check for flagging using declarations not in the inner most namespace

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:22
+  // The target using declaration is either:
+  // 1. not in any namespace declaration, or
+  // 2. in some namespace declaration but not in the innermost layer

Why is this not also checking that the using declaration is not within a header 
file?



Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:24-27
+  Finder->addMatcher(usingDecl(eachOf(
+   usingDecl(unless(hasParent(namespaceDecl(,
+usingDecl(hasParent(namespaceDecl(forEach(namespaceDecl() )
+).bind("use"), this);

Did clang-format produce this? If not,  you should run the patch through 
clang-format.



Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:32
+  const auto *MatchedDecl = Result.Nodes.getNodeAs("use");
+  diag(MatchedDecl->getLocation(), "UsingDecl %0 should be in the innermost "
+"scope. See: https://abseil.io/tips/119";)

aaron.ballman wrote:
> Do not add links to diagnostic messages. They should stand on their own, and 
> not be grammatical with punctuation. I would suggest: `using declaration not 
> declared in the innermost namespace`
I suspect this diagnostic is going to be too aggressive and once you add more 
tests will find it needs some additional constraints.



Comment at: clang-tidy/abseil/SafelyScopedCheck.cpp:32-33
+  const auto *MatchedDecl = Result.Nodes.getNodeAs("use");
+  diag(MatchedDecl->getLocation(), "UsingDecl %0 should be in the innermost "
+"scope. See: https://abseil.io/tips/119";)
+  << MatchedDecl;

Do not add links to diagnostic messages. They should stand on their own, and 
not be grammatical with punctuation. I would suggest: `using declaration not 
declared in the innermost namespace`



Comment at: test/clang-tidy/abseil-safely-scoped.cpp:1
+// RUN: %check_clang_tidy %s abseil-safely-scoped %t
+namespace bar {

You're missing a lot of tests, such as using declarations at function scope, 
within a class scope, etc.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55411/new/

https://reviews.llvm.org/D55411



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

[PATCH] D55409: [clang-tidy] check for using declarations not in an anonymous namespace when there exists one

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp:32
+void AnonymousEnclosedAliasesCheck::check(const MatchFinder::MatchResult 
&Result) {
+
+  const auto *MatchedUsingDecl =

Spurious newline.



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp:41
+// to the vector containing all candidate using declarations.
+if (AnonymousNamespaceDecl) {
+   diag(MatchedUsingDecl->getLocation(),

I don't think this logic works in practice because there's no way to determine 
that the anonymous namespace is even a candidate for putting the using 
declaration into it. Consider a common scenario where there's an anonymous 
namespace declared in a header file (like an STL header outside of the user's 
control), and a using declaration inside of an implementation file. Just 
because the STL declared an anonymous namespace doesn't mean that the user 
could have put their using declaration in it.



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp:43-44
+   diag(MatchedUsingDecl->getLocation(),
+   "UsingDecl %0 should be in the anonymous namespace. 
See: "
+"https://abseil.io/tips/119";)
+   << MatchedUsingDecl;

Diagnostics should not be complete sentences or contain hyperlinks. (Same 
below.)



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h:20
+
+/// Detecting incorrect practice of putting using declarations outside an
+/// anonymous namespace when there exists one.

Detecting incorrect -> Detecting the incorrect



Comment at: clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h:21
+/// Detecting incorrect practice of putting using declarations outside an
+/// anonymous namespace when there exists one.
+/// For the user-facing documentation see:

when there exists one -> when one exists


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55409/new/

https://reviews.llvm.org/D55409



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

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Have you run this check over any large code bases to see what its false 
positive rate looks like? I have to imagine we're going to need some escape 
hatch for system headers (we shouldn't complain about using declarations 
outside of the user's control).




Comment at: clang-tidy/abseil/AliasFreeHeadersCheck.cpp:28
+  const auto *MatchedDecl = Result.Nodes.getNodeAs("x");
+  diag(MatchedDecl->getLocation(), "convenience aliases in header files are "
+"dangerous: see http://google.github.io/styleguide/cppguide.html#Aliases";);

What is a "convenience alias" and why is it dangerous? The diagnostic should 
explain what's wrong with the user's code.



Comment at: clang-tidy/abseil/AliasFreeHeadersCheck.cpp:29
+  diag(MatchedDecl->getLocation(), "convenience aliases in header files are "
+"dangerous: see http://google.github.io/styleguide/cppguide.html#Aliases";);
+}

lebedev.ri wrote:
> http**s**
No links in diagnostics, whether http or https. ;-)

Also, why should this be a separate check from the other positional one in 
D55411?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55410/new/

https://reviews.llvm.org/D55410



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

[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Missing test cases.




Comment at: clang/lib/Sema/SemaType.cpp:5747
+static bool BuildAddressSpaceIndex(LangAS &ASIdx, const Expr *AddrSpace,
+   SourceLocation AttrLoc, Sema &S) {
   if (!AddrSpace->isValueDependent()) {

I think we usually pass the `Sema` argument first in these helper methods.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55447/new/

https://reviews.llvm.org/D55447



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

[PATCH] D54429: [analyzer] Creating standard Sphinx documentation

2018-12-10 Thread Daniel Krupp via Phabricator via cfe-commits

dkrupp added a comment.

@dcoughlin @NoQ ping...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54429/new/

https://reviews.llvm.org/D54429



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added a comment.

Thank you for this fix!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Specify a macro to use instead of ``[[nodiscard]]``. 
+This is useful when maintaining source code that needs to compile with a 
pre-C++17 compiler.

Specifies. Please wrap up to 80 characters.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added a comment.

In D55508#1325316 , @Eugene.Zelenko 
wrote:

> Thank you for this fix!


Your welcome, I was reviewing other revisions to try and get up to speed, and I 
saw you giving someone else the same comment you gave mine!  I'm no python 
expert so this could possible be done a better way, but I hope this at least 
give some relief.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay updated this revision to Diff 177496.
MyDeveloperDay added a comment.

Update the documentation to utilize 80 character lines


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,138 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+// TODO using macros to avoid CHECK-FIXES not being able to find [[xxx]]
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return true;
+}
+
+bool f19() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f19' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:37
+
+Specify a macro to use instead of ``[[nodiscard]]``.  This is useful when 
+maintaining source code that needs to compile with a pre-C++17 compiler.

Option specif**ies**. Please fix double space.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D55510: [ExprConstant] Improve memchr/memcmp for type mismatch and multibyte element types

2018-12-10 Thread Hubert Tong via Phabricator via cfe-commits

hubert.reinterpretcast created this revision.
hubert.reinterpretcast added reviewers: rsmith, aaron.ballman, hfinkel.

`memchr` and `memcmp` operate upon the character units of the object 
representation; that is, the `size_t` parameter expresses the number of 
character units. The constant folding implementation is updated in this patch 
to account for multibyte element types in the arrays passed to 
`memchr`/`memcmp` and, in the case of `memcmp`, to account for the possibility 
that the arrays may have differing element types (even when they are 
byte-sized).

Actual inspection of the object representation is not implemented. Comparisons 
are done only between elements with the same object size; that is, `memchr` 
will fail when inspecting at least one character unit of a multibyte element. 
The integer types are assumed to have two's complement representation with 0 
for `false`, 1 for `true`, and no padding bits.

`memcmp` on multibyte elements will only be able to fold in cases where enough 
elements are equal for the answer to be 0.

CodeGen tests are added for cases that miscompile on some system or other prior 
to this patch.


Repository:
  rC Clang

https://reviews.llvm.org/D55510

Files:
  include/clang/Basic/DiagnosticASTKinds.td
  lib/AST/ExprConstant.cpp
  test/CodeGenCXX/builtins.cpp
  test/SemaCXX/constexpr-string.cpp

Index: test/SemaCXX/constexpr-string.cpp
===
--- test/SemaCXX/constexpr-string.cpp
+++ test/SemaCXX/constexpr-string.cpp
@@ -95,6 +95,51 @@
   static_assert(__builtin_memcmp("abab\0banana", "abab\0canada", 6) == -1);
   static_assert(__builtin_memcmp("abab\0banana", "abab\0canada", 5) == 0);
 
+  extern struct Incomplete incomplete;
+  static_assert(__builtin_memcmp(&incomplete, "", 0u) == 0);
+  static_assert(__builtin_memcmp("", &incomplete, 0u) == 0);
+  static_assert(__builtin_memcmp(&incomplete, "", 1u) == 42); // expected-error {{not an integral constant}} expected-note {{read of incomplete type 'struct Incomplete'}}
+  static_assert(__builtin_memcmp("", &incomplete, 1u) == 42); // expected-error {{not an integral constant}} expected-note {{read of incomplete type 'struct Incomplete'}}
+
+  constexpr unsigned char ku00fe00[] = {0x00, 0xfe, 0x00};
+  constexpr unsigned char ku00feff[] = {0x00, 0xfe, 0xff};
+  constexpr signed char ks00fe00[] = {0, -2, 0};
+  constexpr signed char ks00feff[] = {0, -2, -1};
+  static_assert(__builtin_memcmp(ku00feff, ks00fe00, 2) == 0);
+  static_assert(__builtin_memcmp(ku00feff, ks00fe00, 99) == 1);
+  static_assert(__builtin_memcmp(ku00fe00, ks00feff, 99) == -1);
+  static_assert(__builtin_memcmp(ks00feff, ku00fe00, 2) == 0);
+  static_assert(__builtin_memcmp(ks00feff, ku00fe00, 99) == 1);
+  static_assert(__builtin_memcmp(ks00fe00, ku00feff, 99) == -1);
+  static_assert(__builtin_memcmp(ks00fe00, ks00feff, 2) == 0);
+  static_assert(__builtin_memcmp(ks00feff, ks00fe00, 99) == 1);
+  static_assert(__builtin_memcmp(ks00fe00, ks00feff, 99) == -1);
+
+  constexpr bool kb000100[] = {false, true, false};
+  static_assert(sizeof(bool) != 1u || __builtin_memcmp(ks00fe00, kb000100, 1) == 0);
+  static_assert(sizeof(bool) != 1u || __builtin_memcmp(ks00fe00, kb000100, 2) == 1);
+
+  constexpr long ksl[] = {0, -1};
+  constexpr unsigned int kui[] = {0, 0u - 1};
+  constexpr unsigned long long kull[] = {0, 0ull - 1};
+  constexpr const auto *kuSizeofLong(void) {
+if constexpr(sizeof(long) == sizeof(int)) {
+  return kui;
+} else {
+  static_assert(sizeof(long) == sizeof(long long));
+  return kull;
+}
+  }
+  static_assert(__builtin_memcmp(ksl, kuSizeofLong(), sizeof(long) - 1) == 0);
+  static_assert(__builtin_memcmp(ksl, kuSizeofLong(), sizeof(long) + 0) == 0);
+  static_assert(__builtin_memcmp(ksl, kuSizeofLong(), sizeof(long) + 1) == 0);
+  static_assert(__builtin_memcmp(ksl, kuSizeofLong(), 2*sizeof(long) - 1) == 0);
+  static_assert(__builtin_memcmp(ksl, kuSizeofLong(), 2*sizeof(long) + 0) == 0);
+  static_assert(__builtin_memcmp(ksl, kuSizeofLong(), 2*sizeof(long) + 1) == 42); // expected-error {{not an integral constant}} expected-note {{dereferenced one-past-the-end}}
+  static_assert(__builtin_memcmp(ksl + 1, kuSizeofLong() + 1, sizeof(long) - 1) == 0);
+  static_assert(__builtin_memcmp(ksl + 1, kuSizeofLong() + 1, sizeof(long) + 0) == 0);
+  static_assert(__builtin_memcmp(ksl + 1, kuSizeofLong() + 1, sizeof(long) + 1) == 42); // expected-error {{not an integral constant}} expected-note {{dereferenced one-past-the-end}}
+
   constexpr int a = strcmp("hello", "world"); // expected-error {{constant expression}} expected-note {{non-constexpr function 'strcmp' cannot be used in a constant expression}}
   constexpr int b = strncmp("hello", "world", 3); // expected-error {{constant expression}} expected-note {{non-constexpr function 'strncmp' cannot be used in a constant expression}}
   constexpr int c = memcmp("hello", "world", 3); // expected-error {{consta

Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on Windows

2018-12-10 Thread Aaron Ballman via cfe-commits

On Fri, Dec 7, 2018 at 6:52 PM Stella Stamenova via cfe-commits
 wrote:
>
> Author: stella.stamenova
> Date: Fri Dec  7 15:50:05 2018
> New Revision: 348665
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348665&view=rev
> Log:
> [tests] Fix the FileManagerTest getVirtualFile test on Windows
>
> Summary: The test passes on Windows only when it is executed on the C: drive. 
> If the build and tests run on a different drive, the test is currently 
> failing.
>
> Reviewers: kadircet, asmith
>
> Subscribers: cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D55451
>
> Modified:
> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>
> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=348665&r1=348664&r2=348665&view=diff
> ==
> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Fri Dec  7 15:50:05 2018
> @@ -351,22 +351,34 @@ TEST_F(FileManagerTest, makeAbsoluteUses
>
>  // getVirtualFile should always fill the real path.
>  TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) {
> +  SmallString<64> CustomWorkingDir;
> +#ifdef _WIN32
> +  CustomWorkingDir = "C:/";
> +#else
> +  CustomWorkingDir = "/";
> +#endif

Unfortunately, this is still an issue -- you cannot assume that C:\ is
the correct drive letter on Windows. For instance, this unit test
fails for me because it turns out to be D:\ on my system.

~Aaron

> +
> +  auto FS = IntrusiveRefCntPtr(
> +  new llvm::vfs::InMemoryFileSystem);
> +  // setCurrentworkingdirectory must finish without error.
> +  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(CustomWorkingDir));
> +
> +  FileSystemOptions Opts;
> +  FileManager Manager(Opts, FS);
> +
>// Inject fake files into the file system.
>auto statCache = llvm::make_unique();
>statCache->InjectDirectory("/tmp", 42);
>statCache->InjectFile("/tmp/test", 43);
> -  manager.addStatCache(std::move(statCache));
> +
> +  Manager.addStatCache(std::move(statCache));
>
>// Check for real path.
> -  const FileEntry *file = manager.getVirtualFile("/tmp/test", 123, 1);
> +  const FileEntry *file = Manager.getVirtualFile("/tmp/test", 123, 1);
>ASSERT_TRUE(file != nullptr);
>ASSERT_TRUE(file->isValid());
> -  SmallString<64> ExpectedResult;
> -#ifdef _WIN32
> -  ExpectedResult = "C:/";
> -#else
> -  ExpectedResult = "/";
> -#endif
> +  SmallString<64> ExpectedResult = CustomWorkingDir;
> +
>llvm::sys::path::append(ExpectedResult, "tmp", "test");
>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
>  }
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius added a comment.

Some more minor remarks. I'll give this check a try to see how it behaves on 
some of my projects.
I agree that a high rate of false positives is possible (and is a bit of 
spoiler) but I wouldn't reject this IMO useful check because of that.
Anyway, everything looks pretty clean for me.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //
+  for (const auto *Par : Node.parameters()) {
+const Type *ParType = Par->getType().getTypePtr();

Wouldn't something along the lines:
```
const auto ¶meters = Node.parameters();
return std::any_of(parameters.cbegin(), parameters.cend(), [](const auto *Par) {
  const Type *ParType = Par->getType().getTypePtr();

  if (isNonConstReferenceType(Par->getType())) {
return true;
  }
  if (ParType->isPointerType()) {
return true;
  }
  return false;
});
```
be a little bit more expressive?

I would also refactor it differently:
```
  const Type &ParType = Par->getType(); // not sure about Type

  if (isNonConstReferenceType(ParType)) {
  // ...
  if (ParType.getTypePtr()->isPointerType()) { // is ParType.isPointerType() 
possible?
  // ...
```
but that's a matter of taste.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:12
+or non constant references, and non pointer arguments, and of which the
+member functions are themselve const, have not means of altering any state
+or passing values other than the return type, unless they are using mutable 

themselve -> themselves, have not means -> have no means



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:3
+// RUN:   -config="{CheckOptions: [{key: 
modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+

I'm not sure what guidelines dictate, but I'd love to see another test file 
with `-std=c++14` for instance (or whatever minimal version to necessary to use 
`clang::WarnUnusedResult`, if any).
A very small test would be ok, you can also move some parts of this file into a 
new one.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:63
+   
+// TODO fix CHECK-FIXES to handle "[[" "]]"
+[[nodiscard]] bool f11() const;

You might want to get rid of this `TODO` note now, same for the one on the top 
of the file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

r348762 - Use zip_longest for iterator range comparisons. NFC.

2018-12-10 Thread Michael Kruse via cfe-commits

Author: meinersbur
Date: Mon Dec 10 07:16:37 2018
New Revision: 348762

URL: http://llvm.org/viewvc/llvm-project?rev=348762&view=rev
Log:
Use zip_longest for iterator range comparisons. NFC.

Use zip_longest in two locations that compare iterator ranges.
zip_longest allows the iteration using a range-based for-loop and to be
symmetric over both ranges instead of prioritizing one over the other.
In that latter case code have to handle the case that the first is
longer than the second, the second is longer than the first, and both
are of the same length, which must partially be checked after the loop.

With zip_longest, this becomes an element comparison within the loop
like the comparison of the elements themselves. The symmetry makes it
clearer that neither the first and second iterators are handled
differently. The iterators are not event used directly anymore, just
the ranges.

Differential Revision: https://reviews.llvm.org/D55468

Modified:
cfe/trunk/lib/Sema/SemaOverload.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp

Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=348762&r1=348761&r2=348762&view=diff
==
--- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
+++ cfe/trunk/lib/Sema/SemaOverload.cpp Mon Dec 10 07:16:37 2018
@@ -8977,25 +8977,28 @@ static Comparison compareEnableIfAttrs(c
   auto Cand1Attrs = Cand1->specific_attrs();
   auto Cand2Attrs = Cand2->specific_attrs();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-Cand1ID.clear();
-Cand2ID.clear();
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
 
 // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-// has fewer enable_if attributes than Cand2.
-auto Cand1A = Cand1I++;
-if (Cand1A == Cand1Attrs.end())
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (!Cand1A)
   return Comparison::Worse;
+if (!Cand2A)
+  return Comparison::Better;
+
+Cand1ID.clear();
+Cand2ID.clear();
 
-Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, S.getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,

Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=348762&r1=348761&r2=348762&view=diff
==
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Mon Dec 10 07:16:37 2018
@@ -2913,25 +2913,30 @@ static bool hasSameOverloadableAttrs(con
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != 
BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+
+// Return false if any of the enable_if expressions of A and B are
+// different.
 if (Cand1ID != Cand2ID)
   return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == 
BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.


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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

2018-12-10 Thread Michael Kruse via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Meinersbur marked an inline comment as done.
Closed by commit rC348762: Use zip_longest for iterator range comparisons. NFC. 
(authored by Meinersbur, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55468?vs=177447&id=177506#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55468/new/

https://reviews.llvm.org/D55468

Files:
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,30 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != 
BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+
+// Return false if any of the enable_if expressions of A and B are
+// different.
 if (Cand1ID != Cand2ID)
   return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == 
BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8977,25 +8977,28 @@
   auto Cand1Attrs = Cand1->specific_attrs();
   auto Cand2Attrs = Cand2->specific_attrs();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-Cand1ID.clear();
-Cand2ID.clear();
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
 
 // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-// has fewer enable_if attributes than Cand2.
-auto Cand1A = Cand1I++;
-if (Cand1A == Cand1Attrs.end())
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (!Cand1A)
   return Comparison::Worse;
+if (!Cand2A)
+  return Comparison::Better;
+
+Cand1ID.clear();
+Cand2ID.clear();
 
-Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, S.getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,30 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1I

[PATCH] D53280: [analyzer] Emit an error for invalid -analyzer-config inputs

2018-12-10 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

I guess the solution would be to check whether there are any user supplied 
flags with "analyze" substring, and add the compatibility flag then. It is 
possible if not probable that a non-static-analyzer related flag with a name 
like that will eventually be added, but I guess we can live with it.

The deadline is around the creation of the 8.0.0. release branch, right?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53280/new/

https://reviews.llvm.org/D53280



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

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: lib/Lex/PPExpressions.cpp:154-156
 // Consume the ).
-Result.setEnd(PeekTok.getLocation());
 PP.LexNonComment(PeekTok);
+Result.setEnd(PeekTok.getLocation());

lebedev.ri wrote:
> I'm not sure this is covered with the test?
The tests for that were attached as a file rather than a separate patch. 
Basically, this change is needed to keep the behavior the same for tools like 
pp-trace and modularize. However, I have added new unit tests that demonstrate 
the behavior.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54450/new/

https://reviews.llvm.org/D54450



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

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman updated this revision to Diff 177508.
aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.

Adding tests based on review feedback.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54450/new/

https://reviews.llvm.org/D54450

Files:
  include/clang/Lex/Preprocessor.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  unittests/Lex/PPCallbacksTest.cpp

Index: unittests/Lex/PPCallbacksTest.cpp
===
--- unittests/Lex/PPCallbacksTest.cpp
+++ unittests/Lex/PPCallbacksTest.cpp
@@ -65,6 +65,29 @@
   SrcMgr::CharacteristicKind FileType;
 };
 
+class CondDirectiveCallbacks : public PPCallbacks {
+public:
+  struct Result {
+SourceRange ConditionRange;
+ConditionValueKind ConditionValue;
+
+Result(SourceRange R, ConditionValueKind K)
+: ConditionRange(R), ConditionValue(K) {}
+  };
+
+  std::vector Results;
+
+  void If(SourceLocation Loc, SourceRange ConditionRange,
+  ConditionValueKind ConditionValue) override {
+Results.emplace_back(ConditionRange, ConditionValue);
+  }
+
+  void Elif(SourceLocation Loc, SourceRange ConditionRange,
+ConditionValueKind ConditionValue, SourceLocation IfLoc) override {
+Results.emplace_back(ConditionRange, ConditionValue);
+  }
+};
+
 // Stub to collect data from PragmaOpenCLExtension callbacks.
 class PragmaOpenCLExtensionCallbacks : public PPCallbacks {
 public:
@@ -137,6 +160,15 @@
 return StringRef(B, E - B);
   }
 
+  StringRef GetSourceStringToEnd(CharSourceRange Range) {
+const char *B = SourceMgr.getCharacterData(Range.getBegin());
+const char *E = SourceMgr.getCharacterData(Range.getEnd());
+
+return StringRef(
+B,
+E - B + Lexer::MeasureTokenLength(Range.getEnd(), SourceMgr, LangOpts));
+  }
+
   // Run lexer over SourceText and collect FilenameRange from
   // the InclusionDirective callback.
   CharSourceRange InclusionDirectiveFilenameRange(const char *SourceText,
@@ -199,6 +231,36 @@
 return Callbacks;
   }
 
+  std::vector
+  DirectiveExprRange(StringRef SourceText) {
+TrivialModuleLoader ModLoader;
+MemoryBufferCache PCMCache;
+std::unique_ptr Buf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(Buf)));
+HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+Diags, LangOpts, Target.get());
+Preprocessor PP(std::make_shared(), Diags, LangOpts,
+SourceMgr, PCMCache, HeaderInfo, ModLoader,
+/*IILookup =*/nullptr,
+/*OwnsHeaderSearch =*/false);
+PP.Initialize(*Target);
+auto *Callbacks = new CondDirectiveCallbacks;
+PP.addPPCallbacks(std::unique_ptr(Callbacks));
+
+// Lex source text.
+PP.EnterMainSourceFile();
+
+while (true) {
+  Token Tok;
+  PP.Lex(Tok);
+  if (Tok.is(tok::eof))
+break;
+}
+
+return Callbacks->Results;
+  }
+
   PragmaOpenCLExtensionCallbacks::CallbackParameters
   PragmaOpenCLExtensionCall(const char *SourceText) {
 LangOptions OpenCLLangOpts;
@@ -368,4 +430,59 @@
   ASSERT_EQ(ExpectedState, Parameters.State);
 }
 
-} // anonoymous namespace
+TEST_F(PPCallbacksTest, DirectiveExprRanges) {
+  const auto &Results1 = DirectiveExprRange("#if FLUZZY_FLOOF\n#endif\n");
+  EXPECT_EQ(Results1.size(), 1);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results1[0].ConditionRange, false)),
+  "FLUZZY_FLOOF");
+
+  const auto &Results2 = DirectiveExprRange("#if 1 + 4 < 7\n#endif\n");
+  EXPECT_EQ(Results2.size(), 1);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results2[0].ConditionRange, false)),
+  "1 + 4 < 7");
+
+  const auto &Results3 = DirectiveExprRange("#if 1 + \\\n  2\n#endif\n");
+  EXPECT_EQ(Results3.size(), 1);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results3[0].ConditionRange, false)),
+  "1 + \\\n  2");
+
+  const auto &Results4 = DirectiveExprRange("#if 0\n#elif FLOOFY\n#endif\n");
+  EXPECT_EQ(Results4.size(), 2);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results4[0].ConditionRange, false)),
+  "0");
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results4[1].ConditionRange, false)),
+  "FLOOFY");
+
+  const auto &Results5 = DirectiveExprRange("#if 1\n#elif FLOOFY\n#endif\n");
+  EXPECT_EQ(Results5.size(), 2);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results5[0].ConditionRange, false)),
+  "1");
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results5[1].ConditionRange, false)),
+  "FLOOFY");
+
+  const auto &Results6 =
+  DirectiveExprRange("#if defined(FLUZZY_FLOOF)\n#endif\n");
+  EXPECT_EQ(Results6.size(), 1);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results6[0].ConditionRange, false)),
+  "defined(FLUZZY_FLOOF)");
+
+  const auto &Results7 =
+  DirectiveExprRange("

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

2018-12-10 Thread Gabor Marton via Phabricator via cfe-commits

martong added a comment.

@a_sidorin

The below diff on top of your patch successfully handles the failure with the 
`TestCModules.py` LLDB testcase:

  diff --git a/lib/AST/ASTImporter.cpp b/lib/AST/ASTImporter.cpp
  index 05fec7f943..e6fb590025 100644
  --- a/lib/AST/ASTImporter.cpp
  +++ b/lib/AST/ASTImporter.cpp
  @@ -1695,15 +1695,22 @@ ASTNodeImporter::ImportDeclContext(DeclContext 
*FromDC, bool ForceImport) {
 // LoadFieldsFromExternalStorage().
 auto ImportedDC = import(cast(FromDC));
 assert(ImportedDC);
  -  auto *ToRD = cast(*ImportedDC);
  +  RecordDecl *ToRD = nullptr;
 for (auto *D : FromRD->decls()) {
   if (isa(D) || isa(D)) {
 Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
  -  assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
  -  ToRD->removeDecl(ToD);
  +  if (!ToRD)
  +ToRD = cast(ToD->getDeclContext());
  +  else
  +assert(ToRD == ToD->getDeclContext());
  +  if(ToRD->containsDecl(ToD))
  +ToRD->removeDecl(ToD);
   }
 }
  
  +  if (!ToRD)
  +return Error::success();
  +
 if (!ToRD->hasExternalLexicalStorage())
   assert(ToRD->field_empty());


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44100/new/

https://reviews.llvm.org/D44100



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

2018-12-10 Thread Hyrum Wright via Phabricator via cfe-commits

hwright updated this revision to Diff 177509.
hwright marked 5 inline comments as done.
hwright added a comment.

Use `static_cast` instead of a `switch` for `IndexedMap` lookup.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  clang-tidy/abseil/DurationSubtractionCheck.cpp
  clang-tidy/abseil/DurationSubtractionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-subtraction.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/Inputs/absl/time/time.h
  test/clang-tidy/abseil-duration-comparison.cpp
  test/clang-tidy/abseil-duration-factory-float.cpp
  test/clang-tidy/abseil-duration-factory-scale.cpp
  test/clang-tidy/abseil-duration-subtraction.cpp

Index: test/clang-tidy/abseil-duration-subtraction.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-subtraction.cpp
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s abseil-duration-subtraction %t -- -- -I %S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d, d1;
+
+  x = absl::ToDoubleSeconds(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - d1);
+  x = absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(6.5)) - 8.0;
+  x = absl::ToDoubleHours(d) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleHours(d - absl::Hours(1))
+  x = absl::ToDoubleMinutes(d) - 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d - absl::Minutes(1))
+  x = absl::ToDoubleMilliseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d - absl::Milliseconds(9))
+  x = absl::ToDoubleMicroseconds(d) - 9;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d - absl::Microseconds(9))
+  x = absl::ToDoubleNanoseconds(d) - 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d - absl::Nanoseconds(42))
+
+  // We can rewrite the argument of the duration conversion
+#define THIRTY absl::Seconds(30)
+  x = absl::ToDoubleSeconds(THIRTY) - 1.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: absl::ToDoubleSeconds(THIRTY - absl::Seconds(1))
+#undef THIRTY
+
+  // Some other contexts
+  if (absl::ToDoubleSeconds(d) - 1.0 > 10) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the duration domain [abseil-duration-subtraction]
+  // CHECK-FIXES: if (absl::ToDoubleSeconds(d - absl::Seconds(1)) > 10) {}
+
+  // These should not match
+  x = 5 - 6;
+  x = 4 - absl::ToDoubleSeconds(d) - 6.5 - 8.0;
+  x = absl::ToDoubleSeconds(d) + 1.0;
+  x = absl::ToDoubleSeconds(d) * 1.0;
+  x = absl::ToDoubleSeconds(d) / 1.0;
+
+#define MINUS_FIVE(z) absl::ToDoubleSeconds(z) - 5
+  x = MINUS_FIVE(d);
+#undef MINUS_FIVE
+}
Index: test/clang-tidy/abseil-duration-factory-scale.cpp
===
--- test/clang-tidy/abseil-duration-factory-scale.cpp
+++ test/clang-tidy/abseil-duration-factory-scale.cpp
@@ -1,32 +1,6 @@
-// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t
+// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t -- -- -I %S/Inputs
 
-// Mimic the implementation of absl::Duration
-namespace absl {
-
-class Duration {};
-
-Duration Nanoseconds(long long);
-Duration Microseconds(long long);
-Duration Milliseconds(long long);
-Duration Seconds(long long);
-Duration Minutes(long long);
-Duration Hours(long long);
-
-#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
-  Duration NAME(float n); \
-  Duration NAME(double n);\
-  template\
-  Duration NAME(T n);
-
-GENERATE_DURATION_FACTORY_OVER

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

2018-12-10 Thread Hyrum Wright via Phabricator via cfe-commits

hwright added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:20-39
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {
+switch (Scale) {
+case DurationScale::Hours:
+  return 0;
+case DurationScale::Minutes:

lebedev.ri wrote:
> You should not need this if you change the `enum` instead.
The function is still required; the switch can be removed with a `static_cast`.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:66-77
+InverseMap[DurationScale::Hours] =
+std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours");
+InverseMap[DurationScale::Minutes] =
+std::make_pair("::absl::ToDoubleMinutes", 
"::absl::ToInt64Minutes");
+InverseMap[DurationScale::Seconds] =
+std::make_pair("::absl::ToDoubleSeconds", 
"::absl::ToInt64Seconds");
+InverseMap[DurationScale::Milliseconds] = std::make_pair(

lebedev.ri wrote:
> I was thinking of more like
> ```
> for(std::pair e : std::initializer_list({
>{DurationScale::Hours, 
> std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")}.
>}))
>   InverseMap[e.first] = e.second;
> ```
The compilable construction looks something like:
```
for (const auto &e : {std::make_pair(DurationScale::Hours,
std::make_pair("::absl::ToDoubleHours",
   "::absl::ToInt64Hours"))})
```
Which is a bit more verbose than just assigning values to the map (and not any 
more efficient), so I've just left this bit as-is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

2018-12-10 Thread Gabor Marton via Phabricator via cfe-commits

martong added a comment.

There is another failure on macOS, which is not there on Linux. This is present 
with the 174545 patch id (even before applying my fix for TestCModules).

  $ ninja check-clang-astmerge
  
  Testing Time: 0.63s
  
  Failing Tests (4):
  Clang :: ASTMerge/class-template-partial-spec/test.cpp
  Clang :: ASTMerge/class-template/test.cpp
  Clang :: ASTMerge/class/test.cpp
  Clang :: ASTMerge/struct/test.c
  
Expected Passes: 22
Expected Failures  : 1
Unexpected Failures: 4
  FAILED: tools/clang/test/CMakeFiles/check-clang-astmerge

Now I am diving into this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44100/new/

https://reviews.llvm.org/D44100



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

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

2018-12-10 Thread Arthur O'Dwyer via Phabricator via cfe-commits

Quuxplusone updated this revision to Diff 177510.
Quuxplusone edited the summary of this revision.
Quuxplusone added a comment.

Address review comments.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47344/new/

https://reviews.llvm.org/D47344

Files:
  src/experimental/memory_resource.cpp


Index: src/experimental/memory_resource.cpp
===
--- src/experimental/memory_resource.cpp
+++ src/experimental/memory_resource.cpp
@@ -26,19 +26,20 @@
 class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp
 : public memory_resource
 {
-public:
-~__new_delete_memory_resource_imp() = default;
-
-protected:
-virtual void* do_allocate(size_t __size, size_t __align)
-{ return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */}
+void *do_allocate(size_t size, size_t align) override {
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+if (__is_overaligned_for_new(align))
+__throw_bad_alloc();
+#endif
+return _VSTD::__libcpp_allocate(size, align);
+}
 
-virtual void do_deallocate(void* __p, size_t __n, size_t __align) {
-  _VSTD::__libcpp_deallocate(__p, __n, __align); /* FIXME */
+void do_deallocate(void *p, size_t n, size_t align) override {
+  _VSTD::__libcpp_deallocate(p, n, align);
 }
 
-virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
-{ return &__other == this; }
+bool do_is_equal(memory_resource const & other) const _NOEXCEPT override
+{ return &other == this; }
 };
 
 // null_memory_resource()


Index: src/experimental/memory_resource.cpp
===
--- src/experimental/memory_resource.cpp
+++ src/experimental/memory_resource.cpp
@@ -26,19 +26,20 @@
 class _LIBCPP_TYPE_VIS __new_delete_memory_resource_imp
 : public memory_resource
 {
-public:
-~__new_delete_memory_resource_imp() = default;
-
-protected:
-virtual void* do_allocate(size_t __size, size_t __align)
-{ return _VSTD::__libcpp_allocate(__size, __align); /* FIXME */}
+void *do_allocate(size_t size, size_t align) override {
+#ifdef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+if (__is_overaligned_for_new(align))
+__throw_bad_alloc();
+#endif
+return _VSTD::__libcpp_allocate(size, align);
+}
 
-virtual void do_deallocate(void* __p, size_t __n, size_t __align) {
-  _VSTD::__libcpp_deallocate(__p, __n, __align); /* FIXME */
+void do_deallocate(void *p, size_t n, size_t align) override {
+  _VSTD::__libcpp_deallocate(p, n, align);
 }
 
-virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
-{ return &__other == this; }
+bool do_is_equal(memory_resource const & other) const _NOEXCEPT override
+{ return &other == this; }
 };
 
 // null_memory_resource()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348764 - [libclang] Revert removal of tidy plugin support from libclang introduced in r347496

2018-12-10 Thread Ivan Donchevskii via cfe-commits

Author: yvvan
Date: Mon Dec 10 07:58:50 2018
New Revision: 348764

URL: http://llvm.org/viewvc/llvm-project?rev=348764&view=rev
Log:
[libclang] Revert removal of tidy plugin support from libclang introduced in 
r347496

Differential Revision: https://reviews.llvm.org/D55415

Modified:
cfe/trunk/tools/libclang/CIndex.cpp
cfe/trunk/tools/libclang/CMakeLists.txt

Modified: cfe/trunk/tools/libclang/CIndex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CIndex.cpp?rev=348764&r1=348763&r2=348764&view=diff
==
--- cfe/trunk/tools/libclang/CIndex.cpp (original)
+++ cfe/trunk/tools/libclang/CIndex.cpp Mon Dec 10 07:58:50 2018
@@ -8938,3 +8938,16 @@ cxindex::Logger::~Logger() {
 OS << "--\n";
   }
 }
+
+#ifdef CLANG_TOOL_EXTRA_BUILD
+// This anchor is used to force the linker to link the clang-tidy plugin.
+extern volatile int ClangTidyPluginAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ClangTidyPluginAnchorDestination =
+ClangTidyPluginAnchorSource;
+
+// This anchor is used to force the linker to link the clang-include-fixer
+// plugin.
+extern volatile int ClangIncludeFixerPluginAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ClangIncludeFixerPluginAnchorDestination =
+ClangIncludeFixerPluginAnchorSource;
+#endif

Modified: cfe/trunk/tools/libclang/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CMakeLists.txt?rev=348764&r1=348763&r2=348764&view=diff
==
--- cfe/trunk/tools/libclang/CMakeLists.txt (original)
+++ cfe/trunk/tools/libclang/CMakeLists.txt Mon Dec 10 07:58:50 2018
@@ -47,6 +47,15 @@ if (CLANG_ENABLE_ARCMT)
   list(APPEND LIBS clangARCMigrate)
 endif ()
 
+if (TARGET clangTidyPlugin)
+  add_definitions(-DCLANG_TOOL_EXTRA_BUILD)
+  list(APPEND LIBS clangTidyPlugin)
+  list(APPEND LIBS clangIncludeFixerPlugin)
+  if(LLVM_ENABLE_MODULES)
+list(APPEND LLVM_COMPILE_FLAGS 
"-fmodules-ignore-macro=CLANG_TOOL_EXTRA_BUILD")
+  endif()
+endif ()
+
 find_library(DL_LIBRARY_PATH dl)
 if (DL_LIBRARY_PATH)
   list(APPEND LIBS dl)


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

[PATCH] D55415: Revert removal of tidy plugin support from libclang

2018-12-10 Thread Ivan Donchevskii via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348764: [libclang] Revert removal of tidy plugin support 
from libclang introduced in… (authored by yvvan, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55415?vs=177134&id=177515#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55415/new/

https://reviews.llvm.org/D55415

Files:
  cfe/trunk/tools/libclang/CIndex.cpp
  cfe/trunk/tools/libclang/CMakeLists.txt


Index: cfe/trunk/tools/libclang/CIndex.cpp
===
--- cfe/trunk/tools/libclang/CIndex.cpp
+++ cfe/trunk/tools/libclang/CIndex.cpp
@@ -8938,3 +8938,16 @@
 OS << "--\n";
   }
 }
+
+#ifdef CLANG_TOOL_EXTRA_BUILD
+// This anchor is used to force the linker to link the clang-tidy plugin.
+extern volatile int ClangTidyPluginAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ClangTidyPluginAnchorDestination =
+ClangTidyPluginAnchorSource;
+
+// This anchor is used to force the linker to link the clang-include-fixer
+// plugin.
+extern volatile int ClangIncludeFixerPluginAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ClangIncludeFixerPluginAnchorDestination =
+ClangIncludeFixerPluginAnchorSource;
+#endif
Index: cfe/trunk/tools/libclang/CMakeLists.txt
===
--- cfe/trunk/tools/libclang/CMakeLists.txt
+++ cfe/trunk/tools/libclang/CMakeLists.txt
@@ -47,6 +47,15 @@
   list(APPEND LIBS clangARCMigrate)
 endif ()
 
+if (TARGET clangTidyPlugin)
+  add_definitions(-DCLANG_TOOL_EXTRA_BUILD)
+  list(APPEND LIBS clangTidyPlugin)
+  list(APPEND LIBS clangIncludeFixerPlugin)
+  if(LLVM_ENABLE_MODULES)
+list(APPEND LLVM_COMPILE_FLAGS 
"-fmodules-ignore-macro=CLANG_TOOL_EXTRA_BUILD")
+  endif()
+endif ()
+
 find_library(DL_LIBRARY_PATH dl)
 if (DL_LIBRARY_PATH)
   list(APPEND LIBS dl)


Index: cfe/trunk/tools/libclang/CIndex.cpp
===
--- cfe/trunk/tools/libclang/CIndex.cpp
+++ cfe/trunk/tools/libclang/CIndex.cpp
@@ -8938,3 +8938,16 @@
 OS << "--\n";
   }
 }
+
+#ifdef CLANG_TOOL_EXTRA_BUILD
+// This anchor is used to force the linker to link the clang-tidy plugin.
+extern volatile int ClangTidyPluginAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ClangTidyPluginAnchorDestination =
+ClangTidyPluginAnchorSource;
+
+// This anchor is used to force the linker to link the clang-include-fixer
+// plugin.
+extern volatile int ClangIncludeFixerPluginAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ClangIncludeFixerPluginAnchorDestination =
+ClangIncludeFixerPluginAnchorSource;
+#endif
Index: cfe/trunk/tools/libclang/CMakeLists.txt
===
--- cfe/trunk/tools/libclang/CMakeLists.txt
+++ cfe/trunk/tools/libclang/CMakeLists.txt
@@ -47,6 +47,15 @@
   list(APPEND LIBS clangARCMigrate)
 endif ()
 
+if (TARGET clangTidyPlugin)
+  add_definitions(-DCLANG_TOOL_EXTRA_BUILD)
+  list(APPEND LIBS clangTidyPlugin)
+  list(APPEND LIBS clangIncludeFixerPlugin)
+  if(LLVM_ENABLE_MODULES)
+list(APPEND LLVM_COMPILE_FLAGS "-fmodules-ignore-macro=CLANG_TOOL_EXTRA_BUILD")
+  endif()
+endif ()
+
 find_library(DL_LIBRARY_PATH dl)
 if (DL_LIBRARY_PATH)
   list(APPEND LIBS dl)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth added inline comments.



Comment at: clang-tidy/add_new_check.py:213
 match = re.search('Improvements to clang-tidy', line)
+match_next = re.search('Improvements to include-fixer', line)
+match_checker = re.search('- New :doc:`(.*)', line)

I think it would be better to compile the regular expression and match with the 
compiled version.



Comment at: clang-tidy/add_new_check.py:218
+  if last_checker > check_name_dashes:
+add_note_here=True
+

Please add whitespace around `=`



Comment at: clang-tidy/add_new_check.py:233
+
+if header_found & add_note_here:
   if not line.startswith(''):

what do you mean with the `&`? Is this  intended as bit-operator?

a `if header_found and add_note_here:` is better for logical expressions in 
python.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508



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

[PATCH] D55482: [clang-tidy] Improve google-objc-function-naming diagnostics 📙

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang-tidy/google/FunctionNamingCheck.cpp:114-125
+  if (MatchedDecl->getStorageClass() == SC_Static) {
+diag(MatchedDecl->getLocation(),
+ "static function name %0 must be in Pascal case as required by "
+ "Google Objective-C style guide")
+<< MatchedDecl << generateFixItHint(MatchedDecl);
+return;
+  }

I'd rather see these diagnostics combined: `%select{static|global}1 function 
name %0 must %select{be in |have an appropriate prefixed followed by "}1 Pascal 
case as required by the Google Objective-C style guide` to simplify the logic 
(since `generateFixItHint()` already does the right thing).


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55482/new/

https://reviews.llvm.org/D55482



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

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

2018-12-10 Thread Alexey Bataev via Phabricator via cfe-commits

ABataev added inline comments.



Comment at: lib/AST/ASTDumper.cpp:1056
 }
+NodeDumper.dumpPointer(Initializer);
+  }

Better to output it immediately after `initializer` keyword.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55395/new/

https://reviews.llvm.org/D55395



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

[PATCH] D55212: Handle alloc_size attribute on function pointers

2018-12-10 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson updated this revision to Diff 177517.
arichardson added a comment.

clang-format
fix a failing test


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55212/new/

https://reviews.llvm.org/D55212

Files:
  include/clang/Basic/Attr.td
  lib/AST/ExprConstant.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/alloc-size-fnptr.c
  test/CodeGen/alloc-size.c
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/Sema/alloc-size.c
  test/SemaCXX/alloc-size.cpp

Index: test/SemaCXX/alloc-size.cpp
===
--- /dev/null
+++ test/SemaCXX/alloc-size.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 %s -verify
+
+class Test {
+public:
+  int alloc(int) __attribute__((alloc_size(2)));// expected-warning{{'alloc_size' attribute only applies to return values that are pointers}}
+  void *alloc2(int) __attribute__((alloc_size(1))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void *alloc3(int) __attribute__((alloc_size(2))); // fine
+  void *alloc4(int) __attribute__((alloc_size(3))); // expected-error{{'alloc_size' attribute parameter 1 is out of bounds}}
+  void *alloc5(int, int) __attribute__((alloc_size(1, 2))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void *alloc6(int, int) __attribute__((alloc_size(2, 1))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void *alloc7(int, int) __attribute__((alloc_size(2, 3))); // fine
+  // cannot overload based on alloc_size attribute:
+  void *overload(int, int) __attribute__((alloc_size(2, 3))); // expected-note{{previous declaration is here}}
+  void *overload(int, int);   // expected-error{{class member cannot be redeclared}}
+};
+// Check that we can't overload functions based on alloc_size on parameters:
+void test1(void *(__attribute__((alloc_size(1))) * fn_ptr_with_attr)(int)) {} // expected-note{{previous definition is here}}
+void test1(void *(*fn_ptr_without_attr)(int)) {}  // expected-error {{redefinition of 'test1'}}
+
+// We should not be warning when assigning function pointers with and without the alloc size attribute
+// since it doesn't change the type of the function
+typedef void *(__attribute__((alloc_size(1))) * my_malloc_fn_pointer_type)(int);
+typedef void *(*my_other_malloc_fn_pointer_type)(int);
+void *fn(int i);
+__attribute__((alloc_size(1))) void *fn2(int i);
+
+int main() {
+  my_malloc_fn_pointer_type f = fn;
+  my_other_malloc_fn_pointer_type f2 = fn;
+  my_malloc_fn_pointer_type f3 = fn2;
+  my_other_malloc_fn_pointer_type f4 = fn2;
+}
Index: test/Sema/alloc-size.c
===
--- test/Sema/alloc-size.c
+++ test/Sema/alloc-size.c
@@ -14,7 +14,7 @@
 
 int fail9(int a) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to return values that are pointers}}
 
-int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
+int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to non-K&R-style functions}}
 
 void *fail11(void *a) __attribute__((alloc_size(1))); //expected-error{{'alloc_size' attribute argument may only refer to a function parameter of integer type}}
 
@@ -22,4 +22,39 @@
 void *fail12(int a) __attribute__((alloc_size(1, "abc"))); //expected-error{{'alloc_size' attribute requires parameter 2 to be an integer constant}}
 void *fail13(int a) __attribute__((alloc_size(1U<<31))); //expected-error{{integer constant expression evaluates to value 2147483648 that cannot be represented in a 32-bit signed integer type}}
 
-int (*PR31453)(int) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
+void *(*PR31453)(int)__attribute__((alloc_size(1)));
+
+void *KR() __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to non-K&R-style functions}}
+
+// Applying alloc_size to function pointers should work:
+void *(__attribute__((alloc_size(1))) * func_ptr1)(int);
+void *(__attribute__((alloc_size(1, 2))) func_ptr2)(int, int);
+
+// TODO: according to GCC documentation the following should actually be the type
+// “pointer to pointer to alloc_size attributed function returning void*” and should
+// therefore be supported
+void *(__attribute__((alloc_size(1))) * *ptr_to_func_ptr)(int); // expected-warning{{'alloc_size' attribute only applies to non-K&R-style functions}}
+// The following definitions apply the attribute to the pointer to the function pointer which should not be possible
+void *(*__attribute__((alloc_size(1))) * ptr_to_func_ptr2)(int); // expected-warning{{'alloc_size' attribute only applies to non-K&R-style functions}}
+void *(*

[PATCH] D55212: Handle alloc_size attribute on function pointers

2018-12-10 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson updated this revision to Diff 177519.
arichardson marked 3 inline comments as done.
arichardson added a comment.

fix typo


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55212/new/

https://reviews.llvm.org/D55212

Files:
  include/clang/Basic/Attr.td
  lib/AST/ExprConstant.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/alloc-size-fnptr.c
  test/CodeGen/alloc-size.c
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/Sema/alloc-size.c
  test/SemaCXX/alloc-size.cpp

Index: test/SemaCXX/alloc-size.cpp
===
--- /dev/null
+++ test/SemaCXX/alloc-size.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 %s -verify
+
+class Test {
+public:
+  int alloc(int) __attribute__((alloc_size(2)));// expected-warning{{'alloc_size' attribute only applies to return values that are pointers}}
+  void *alloc2(int) __attribute__((alloc_size(1))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void *alloc3(int) __attribute__((alloc_size(2))); // fine
+  void *alloc4(int) __attribute__((alloc_size(3))); // expected-error{{'alloc_size' attribute parameter 1 is out of bounds}}
+  void *alloc5(int, int) __attribute__((alloc_size(1, 2))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void *alloc6(int, int) __attribute__((alloc_size(2, 1))); // expected-error{{'alloc_size' attribute is invalid for the implicit this argument}}
+  void *alloc7(int, int) __attribute__((alloc_size(2, 3))); // fine
+  // cannot overload based on alloc_size attribute:
+  void *overload(int, int) __attribute__((alloc_size(2, 3))); // expected-note{{previous declaration is here}}
+  void *overload(int, int);   // expected-error{{class member cannot be redeclared}}
+};
+// Check that we can't overload functions based on alloc_size on parameters:
+void test1(void *(__attribute__((alloc_size(1))) * fn_ptr_with_attr)(int)) {} // expected-note{{previous definition is here}}
+void test1(void *(*fn_ptr_without_attr)(int)) {}  // expected-error {{redefinition of 'test1'}}
+
+// We should not be warning when assigning function pointers with and without the alloc size attribute
+// since it doesn't change the type of the function.
+typedef void *(__attribute__((alloc_size(1))) * my_malloc_fn_pointer_type)(int);
+typedef void *(*my_other_malloc_fn_pointer_type)(int);
+void *fn(int i);
+__attribute__((alloc_size(1))) void *fn2(int i);
+
+int main() {
+  my_malloc_fn_pointer_type f = fn;
+  my_other_malloc_fn_pointer_type f2 = fn;
+  my_malloc_fn_pointer_type f3 = fn2;
+  my_other_malloc_fn_pointer_type f4 = fn2;
+}
Index: test/Sema/alloc-size.c
===
--- test/Sema/alloc-size.c
+++ test/Sema/alloc-size.c
@@ -14,7 +14,7 @@
 
 int fail9(int a) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to return values that are pointers}}
 
-int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
+int fail10 __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to non-K&R-style functions}}
 
 void *fail11(void *a) __attribute__((alloc_size(1))); //expected-error{{'alloc_size' attribute argument may only refer to a function parameter of integer type}}
 
@@ -22,4 +22,39 @@
 void *fail12(int a) __attribute__((alloc_size(1, "abc"))); //expected-error{{'alloc_size' attribute requires parameter 2 to be an integer constant}}
 void *fail13(int a) __attribute__((alloc_size(1U<<31))); //expected-error{{integer constant expression evaluates to value 2147483648 that cannot be represented in a 32-bit signed integer type}}
 
-int (*PR31453)(int) __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to functions}}
+void *(*PR31453)(int)__attribute__((alloc_size(1)));
+
+void *KR() __attribute__((alloc_size(1))); //expected-warning{{'alloc_size' attribute only applies to non-K&R-style functions}}
+
+// Applying alloc_size to function pointers should work:
+void *(__attribute__((alloc_size(1))) * func_ptr1)(int);
+void *(__attribute__((alloc_size(1, 2))) func_ptr2)(int, int);
+
+// TODO: according to GCC documentation the following should actually be the type
+// “pointer to pointer to alloc_size attributed function returning void*” and should
+// therefore be supported
+void *(__attribute__((alloc_size(1))) * *ptr_to_func_ptr)(int); // expected-warning{{'alloc_size' attribute only applies to non-K&R-style functions}}
+// The following definitions apply the attribute to the pointer to the function pointer which should not be possible
+void *(*__attribute__((alloc_size(1))) * ptr_to_func_ptr2)(int); // expected-warning{{'alloc_size' attribute only applies to non-K&R-sty

[PATCH] D55212: Handle alloc_size attribute on function pointers

2018-12-10 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson marked 5 inline comments as done.
arichardson added inline comments.



Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:15
+// FIXME: After changing the subject from Function to HasFunctionProto, 
AllocSize is no longer listed (similar to Format, etc)
+// FIXME-NEXT: AllocSize (SubjectMatchRule_function)
 // CHECK-NEXT: AlwaysDestroy (SubjectMatchRule_variable)

This seems to also affect __attribute((format)) and others so I'm not sure 
whether removing AllocSize from this list is a blocker for this patch.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55212/new/

https://reviews.llvm.org/D55212



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

[PATCH] D55245: [clang-tidy] Add the abseil-duration-subtraction check

2018-12-10 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:21
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {

Are you using `argument_type`? Browser searching did only show one result.



Comment at: clang-tidy/abseil/DurationRewriter.cpp:23
+  unsigned operator()(DurationScale Scale) const {
+return static_cast(Scale);
+  }

Why not `std::uint8_t` as its the underlying type for the `enum`?



Comment at: clang-tidy/abseil/DurationRewriter.cpp:73
+static llvm::Optional
+maybeRewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
+DurationScale Scale, const Expr &Node) {

`maybe` in the name is redundant, as its return type is `Optional`



Comment at: clang-tidy/abseil/DurationRewriter.cpp:77
+  getInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst(
+  "e",

In Principle the `Node` could have multiple expressions that are a call if 
there is nesting. 
The transformation is correct from what I see right now, but might result in 
the necessity of multiple passes for the check. (Is the addition version 
affected from that too?)

Given the recursive nature of the matcher you could construct a nesting with 
the inner part being a subtraction, too. The generated fixes would conflict and 
none of them would be applied. At least thats what I would expect right now. 
Please take a look at this issue.



Comment at: test/clang-tidy/Inputs/absl/time/time.h:1
+// Mimic the implementation of absl::Duration
+namespace absl {

I think having the extraction of the common test-stuff into this header as one 
commit would be better. Would you prepare such a patch? I can commit for you. 
It probably makes sense if you ask for commit access 
(https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). Do as you 
wish.



Comment at: test/clang-tidy/abseil-duration-subtraction.cpp:12
+  // CHECK-FIXES: absl::ToDoubleSeconds(d - absl::Seconds(1))
+  x = absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the 
duration domain [abseil-duration-subtraction]

From this example starting:

- The RHS should be a nested expression with function calls, as the RHS is 
transformed to create the adversary example i mean in the transformation 
function above.

```
absl::ToDoubleSeconds(d) - absl::ToDoubleSeconds(absl::ToDoubleSeconds(d) - 
absl::ToDoubleSeconds(d1));
```
I think you need the proper conversion function, as the result of the 
expression is `double` and you need a `Duration`, right?

But in principle starting from this idea the transformation might break.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55245/new/

https://reviews.llvm.org/D55245



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius requested changes to this revision.
curdeius added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //
+  for (const auto *Par : Node.parameters()) {
+const Type *ParType = Par->getType().getTypePtr();

curdeius wrote:
> Wouldn't something along the lines:
> ```
> const auto ¶meters = Node.parameters();
> return std::any_of(parameters.cbegin(), parameters.cend(), [](const auto 
> *Par) {
>   const Type *ParType = Par->getType().getTypePtr();
> 
>   if (isNonConstReferenceType(Par->getType())) {
> return true;
>   }
>   if (ParType->isPointerType()) {
> return true;
>   }
>   return false;
> });
> ```
> be a little bit more expressive?
> 
> I would also refactor it differently:
> ```
>   const Type &ParType = Par->getType(); // not sure about Type
> 
>   if (isNonConstReferenceType(ParType)) {
>   // ...
>   if (ParType.getTypePtr()->isPointerType()) { // is ParType.isPointerType() 
> possible?
>   // ...
> ```
> but that's a matter of taste.
I've just seen that you can use `.param_begin()` and `.param_end()` instead of 
`parameters.begin()/end()`.
I'd also reduce the use of `auto` here, because it's pretty hard to read it IMO.
So, I'd suggest

```
return std::any_of(Node.param_begin(), Node.param_end(), [](const ParmVarDecl 
*Par) {
  const QualType &ParType = Par->getType();

  if (isNonConstReferenceType(ParType)) {
return true;
  }
  if (ParType.getTypePtr()->isPointerType()) {
return true;
  }
  return false;
});
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay updated this revision to Diff 177532.
MyDeveloperDay marked an inline comment as done.
MyDeveloperDay added a comment.

Addressing review comments,

- additional unit tests for no ReplacementString and C++ 11 case
- more expressive hasNonConstReferenceOrPointerArguments matcher
- minor documentation typos and spacing


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseNodiscardCheck.cpp
  clang-tidy/modernize/UseNodiscardCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-nodiscard.rst
  test/clang-tidy/modernize-use-nodiscard-cxx11.cpp
  test/clang-tidy/modernize-use-nodiscard-no-macro.cpp
  test/clang-tidy/modernize-use-nodiscard.cpp

Index: test/clang-tidy/modernize-use-nodiscard.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-nodiscard.cpp
@@ -0,0 +1,136 @@
+// RUN: %check_clang_tidy %s modernize-use-nodiscard %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.ReplacementString, value: 'NO_DISCARD'}]}" \
+// RUN: -- -std=c++17 \
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define NO_DISCARD [[nodiscard]]
+#define NO_RETURN [[noreturn]]
+
+class Foo
+{
+public:
+bool f1() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f1' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f1() const;
+
+bool f2(int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f2' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f2(int) const;
+
+bool f3(const int &) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f3' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f3(const int &) const;
+
+bool f4(void) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f4' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f4(void) const;
+
+// negative tests
+
+void f5() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: void f5() const;
+
+bool f6();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f6();
+
+bool f7(int &);
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f7(int &);
+
+bool f8(int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f8(int &) const;
+
+bool f9(int *) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f9(int *) const;
+
+bool f10(const int &,int &) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool f10(const int &,int &) const;
+
+NO_DISCARD bool f12() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_DISCARD bool f12() const;
+
+MUST_USE_RESULT bool f13() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: MUST_USE_RESULT bool f13() const;
+   
+[[nodiscard]] bool f11() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: {{\[\[nodiscard\]\]}} bool f11() const;
+
+bool _f20() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function '_f20' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool _f20() const;
+
+NO_RETURN bool f21() const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: NO_RETURN bool f21() const;
+
+~Foo();
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: ~Foo();
+
+bool operator +=(int) const;
+// CHECK-MESSAGES-NOT: warning:
+// CHECK-FIXES: bool operator +=(int) const;
+   
+// extra keywords (virtual,inline,const) on return type
+
+virtual bool f14() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f14' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD virtual bool f14() const;
+
+const bool f15() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f15' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD const bool f15() const;
+
+inline const bool f16() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f16' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline const bool f16() const;
+
+inline virtual const bool f17() const;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f17' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD inline virtual const bool f17() const;
+
+// inline with body
+bool f18() const {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f18' should be marked NO_DISCARD [modernize-use-nodiscard]
+// CHECK-FIXES: NO_DISCARD bool f18() const {
+return tr

[PATCH] D47757: [Sema] Produce diagnostics when unavailable aligned allocation/deallocation functions are called

2018-12-10 Thread Akira Hatanaka via Phabricator via cfe-commits

ahatanak added a comment.

ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47757/new/

https://reviews.llvm.org/D47757



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius accepted this revision.
curdeius added a comment.
This revision is now accepted and ready to land.

LGTM. But I'm not a code owner here and I don't know if you need an acceptance 
of one of them.
Great job.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

2018-12-10 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

I can't fix these right away, but I don't want myself to forget it before 
commiting. Pay no attention.




Comment at: include/clang/StaticAnalyzer/Checkers/CheckerBase.td:51-56
+/// relies on information MallocBase gathers.
+/// Example:
+///   def InnerPointerChecker : Checker<"InnerPointer">,
+/// HelpText<"Check for inner pointers of C++ containers used after "
+///  "re/deallocation">,
+/// Dependencies<[MallocBase]>;

`MallocBase` was renamed to `DynamicMemoryModeling`



Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:104
 StringRef Desc;
+llvm::SmallVector Dependencies;
 

Should be `ConstCheckerInfoList`.



Comment at: include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:139
 
+  /// Makes the checker with the full name \p fullName depends on the checker
+  /// called \p dependency.

typo: depend



Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:174
+
+assert(std::is_sorted(Checkers.cbegin(), Checkers.cend(), checkerNameLT));
+

The same assert in there just a couple lines above.



Comment at: utils/TableGen/ClangSACheckersEmitter.cpp:102-103
+  //   alpha.cplusplus.UninitializedObject
+  //   - CLASS: The name of the checker, with "Checker" appended, e.g.:
+  //UninitializedObjectChecker
+  //   - HELPTEXT: The description of the checker.

This is incorrect.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54438/new/

https://reviews.llvm.org/D54438



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay updated this revision to Diff 177538.
MyDeveloperDay added a comment.

Fix review comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508

Files:
  clang-tidy/add_new_check.py


Index: clang-tidy/add_new_check.py
===
--- clang-tidy/add_new_check.py
+++ clang-tidy/add_new_check.py
@@ -200,23 +200,47 @@
   with open(filename, 'r') as f:
 lines = f.readlines()
 
+  lineMatcher = re.compile('Improvements to clang-tidy')
+  nextSectionMatcher = re.compile('Improvements to include-fixer')
+  checkerMatcher = re.compile('- New :doc:`(.*)')
+
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
 note_added = False
 header_found = False
+next_header_found = False
+add_note_here = False
 
 for line in lines:
   if not note_added:
-match = re.search('Improvements to clang-tidy', line)
+match = lineMatcher.match(line)
+match_next = nextSectionMatcher.match(line)
+match_checker = checkerMatcher.match(line)
+if match_checker:
+  last_checker = match_checker.group(1)
+  if last_checker > check_name_dashes:
+add_note_here = True
+
+if match_next:
+  next_header_found = True
+  add_note_here = True
+
 if match:
   header_found = True
-elif header_found:
+  f.write(line)
+  continue
+
+if line.startswith(''):
+  f.write(line)
+  continue
+
+if header_found and add_note_here:
   if not line.startswith(''):
-f.write("""
-- New :doc:`%s
+f.write("""- New :doc:`%s
   ` check.
 
   FIXME: add release notes.
+
 """ % (check_name_dashes, check_name_dashes))
 note_added = True
 


Index: clang-tidy/add_new_check.py
===
--- clang-tidy/add_new_check.py
+++ clang-tidy/add_new_check.py
@@ -200,23 +200,47 @@
   with open(filename, 'r') as f:
 lines = f.readlines()
 
+  lineMatcher = re.compile('Improvements to clang-tidy')
+  nextSectionMatcher = re.compile('Improvements to include-fixer')
+  checkerMatcher = re.compile('- New :doc:`(.*)')
+
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
 note_added = False
 header_found = False
+next_header_found = False
+add_note_here = False
 
 for line in lines:
   if not note_added:
-match = re.search('Improvements to clang-tidy', line)
+match = lineMatcher.match(line)
+match_next = nextSectionMatcher.match(line)
+match_checker = checkerMatcher.match(line)
+if match_checker:
+  last_checker = match_checker.group(1)
+  if last_checker > check_name_dashes:
+add_note_here = True
+
+if match_next:
+  next_header_found = True
+  add_note_here = True
+
 if match:
   header_found = True
-elif header_found:
+  f.write(line)
+  continue
+
+if line.startswith(''):
+  f.write(line)
+  continue
+
+if header_found and add_note_here:
   if not line.startswith(''):
-f.write("""
-- New :doc:`%s
+f.write("""- New :doc:`%s
   ` check.
 
   FIXME: add release notes.
+
 """ % (check_name_dashes, check_name_dashes))
 note_added = True
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added a comment.

@curdeius Thanks, I don't have commit access so I'm happy wait for a 
CODE_OWNER, they could likely have more input.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D47111: : Implement monotonic_buffer_resource.

2018-12-10 Thread Arthur O'Dwyer via Phabricator via cfe-commits

Quuxplusone updated this revision to Diff 177540.
Quuxplusone marked 5 inline comments as done.
Quuxplusone added a comment.

@ericwf ping!


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47111/new/

https://reviews.llvm.org/D47111

Files:
  include/experimental/memory_resource
  src/experimental/memory_resource.cpp
  
test/libcxx/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/copy_move.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/with_default_resource.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.ctor/without_buffer.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_deallocate.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_initial_buffer.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_underaligned_buffer.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_from_zero_sized_buffer.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_with_initial_size.pass.cpp
  
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp

Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/equality.pass.cpp
@@ -0,0 +1,62 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// REQUIRES: c++experimental
+// UNSUPPORTED: c++98, c++03
+
+// 
+
+// class monotonic_buffer_resource
+
+#include 
+#include 
+#include 
+#include 
+
+struct assert_on_compare : public std::experimental::pmr::memory_resource
+{
+protected:
+void *do_allocate(size_t, size_t)
+{ assert(false); }
+
+void do_deallocate(void *, size_t, size_t)
+{ assert(false); }
+
+bool do_is_equal(std::experimental::pmr::memory_resource const &) const noexcept
+{ assert(false); }
+};
+
+int main()
+{
+// Same object
+{
+std::experimental::pmr::monotonic_buffer_resource r1;
+std::experimental::pmr::monotonic_buffer_resource r2;
+assert(r1 == r1);
+assert(r1 != r2);
+
+std::experimental::pmr::memory_resource & p1 = r1;
+std::experimental::pmr::memory_resource & p2 = r2;
+assert(p1 == p1);
+assert(p1 != p2);
+assert(p1 == r1);
+assert(r1 == p1);
+assert(p1 != r2);
+assert(r2 != p1);
+}
+// Different types
+{
+std::experimental::pmr::monotonic_buffer_resource mono1;
+std::experimental::pmr::memory_resource & r1 = mono1;
+assert_on_compare c;
+std::experimental::pmr::memory_resource & r2 = c;
+assert(r1 != r2);
+assert(!(r1 == r2));
+}
+}
Index: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_with_initial_size.pass.cpp
===
--- /dev/null
+++ test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_with_initial_size.pass.cpp
@@ -0,0 +1,51 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// REQUIRES: c++experimental
+// UNSUPPORTED: c++98, c++03
+
+// 
+
+// class monotonic_buffer_resource
+
+#include 
+#include 
+
+#include "count_new.hpp"
+
+void test(size_t initial_buffer_size)
+{
+globalMemCounter.reset();
+
+std::experimental::pmr::monotonic_buffer_resource mono1(
+initial_buffer_size,
+std::experimental::pmr::new_d

RE: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on Windows

2018-12-10 Thread Stella Stamenova via cfe-commits

Our tests run on drive E:\ (not C:\) which is why we saw this test failing. 
After this change, the test can now run successfully for us because the 
temporary files are created and checked for on the C:\ drive. Before this 
change, the temporary files were created on the E:\ drive and checked for on 
the C:\ drive.

Are you saying that you have a drive C:\ and the test is failing anyway? Or do 
you not even have a drive C:\?

Thanks,
-Stella

-Original Message-
From: Aaron Ballman  
Sent: Monday, December 10, 2018 6:55 AM
To: Stella Stamenova 
Cc: cfe-commits 
Subject: Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on 
Windows

On Fri, Dec 7, 2018 at 6:52 PM Stella Stamenova via cfe-commits 
 wrote:
>
> Author: stella.stamenova
> Date: Fri Dec  7 15:50:05 2018
> New Revision: 348665
>
> URL: 
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.
> org%2Fviewvc%2Fllvm-project%3Frev%3D348665%26view%3Drev&data=02%7C
> 01%7Cstilis%40microsoft.com%7Cc4e90b6970994d2b15b608d65eaf7ff3%7C72f98
> 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C636800505193880574&sdata=vv0
> qe64wy0oqci7RAlCXHxmjREevqS3s%2ByNz83tqQIw%3D&reserved=0
> Log:
> [tests] Fix the FileManagerTest getVirtualFile test on Windows
>
> Summary: The test passes on Windows only when it is executed on the C: drive. 
> If the build and tests run on a different drive, the test is currently 
> failing.
>
> Reviewers: kadircet, asmith
>
> Subscribers: cfe-commits
>
> Differential Revision: 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Frevi
> ews.llvm.org%2FD55451&data=02%7C01%7Cstilis%40microsoft.com%7Cc4e9
> 0b6970994d2b15b608d65eaf7ff3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C
> 0%7C636800505193880574&sdata=0dFS4XR2o85FyI0XHr9Eh4pX%2BbdC2CPhXdd
> X3lDYaao%3D&reserved=0
>
> Modified:
> cfe/trunk/unittests/Basic/FileManagerTest.cpp
>
> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> URL: 
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.
> org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Funittests%2FBasic%2FFileMa
> nagerTest.cpp%3Frev%3D348665%26r1%3D348664%26r2%3D348665%26view%3Ddiff
> &data=02%7C01%7Cstilis%40microsoft.com%7Cc4e90b6970994d2b15b608d65
> eaf7ff3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63680050519388057
> 4&sdata=UkdKXFFYeXogiCIJ8%2B41qUFO2qON9seRUVYziL2%2B9Yg%3D&res
> erved=0 
> ==
> 
> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Fri Dec  7 15:50:05 
> +++ 2018
> @@ -351,22 +351,34 @@ TEST_F(FileManagerTest, makeAbsoluteUses
>
>  // getVirtualFile should always fill the real path.
>  TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) {
> +  SmallString<64> CustomWorkingDir;
> +#ifdef _WIN32
> +  CustomWorkingDir = "C:/";
> +#else
> +  CustomWorkingDir = "/";
> +#endif

Unfortunately, this is still an issue -- you cannot assume that C:\ is the 
correct drive letter on Windows. For instance, this unit test fails for me 
because it turns out to be D:\ on my system.

~Aaron

> +
> +  auto FS = IntrusiveRefCntPtr(
> +  new llvm::vfs::InMemoryFileSystem);  // 
> + setCurrentworkingdirectory must finish without error.
> +  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(CustomWorkingDir));
> +
> +  FileSystemOptions Opts;
> +  FileManager Manager(Opts, FS);
> +
>// Inject fake files into the file system.
>auto statCache = llvm::make_unique();
>statCache->InjectDirectory("/tmp", 42);
>statCache->InjectFile("/tmp/test", 43);
> -  manager.addStatCache(std::move(statCache));
> +
> +  Manager.addStatCache(std::move(statCache));
>
>// Check for real path.
> -  const FileEntry *file = manager.getVirtualFile("/tmp/test", 123, 
> 1);
> +  const FileEntry *file = Manager.getVirtualFile("/tmp/test", 123, 
> + 1);
>ASSERT_TRUE(file != nullptr);
>ASSERT_TRUE(file->isValid());
> -  SmallString<64> ExpectedResult;
> -#ifdef _WIN32
> -  ExpectedResult = "C:/";
> -#else
> -  ExpectedResult = "/";
> -#endif
> +  SmallString<64> ExpectedResult = CustomWorkingDir;
> +
>llvm::sys::path::append(ExpectedResult, "tmp", "test");
>EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);  }
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fcfe-commits&data=02%7C01%7Cstilis%40microsoft.com%7Cc4e90b6970994d2b15b608d65eaf7ff3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636800505193880574&sdata=LRq7kLE%2BrPavTkQ2gSnc%2B%2FHrPMc8V1QQLWKr2gIPSBs%3D&reserved=0
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth added a comment.

I think this patch is fine, AFAIK these utility scripts are not tested directly 
but are just adjusted if they dont work as expected :)

Did you test it with a fake new-check? If it does what we expect its fine :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-10 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth added a comment.

Hi MyDeveloperDay,

thanks for the patch! Mostly stylistic comments. Would it make sense to attach 
the attribute to the implementation of the functions too?

This check is definitly useful, but noisy. Do you see a change of another 
heuristic that could be applied to reduce noise? Did you run the check over 
LLVM as this is our common experiment.




Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:29
+AST_MATCHER(CXXMethodDecl, isOverloadedOperator) {
+  // don't put [[nodiscard]] front of operators
+  return Node.isOverloadedOperator();

Please make this comment a full sentence with punctuation and correct 
grammar/spelling. Same with other comments.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:45
+  //bool foo()
+  return std::any_of(Node.param_begin(), Node.param_end(), [](const 
ParmVarDecl *Par) {
+const QualType &ParType = Par->getType();

you can take `llvm::any_of(Node.parameters(), ...)` as a range-based version of 
the algorithm.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:46
+  return std::any_of(Node.param_begin(), Node.param_end(), [](const 
ParmVarDecl *Par) {
+const QualType &ParType = Par->getType();
+

`QualType` is usually used as value, because its small. Same above. Once its a 
value, please ellide the `const` as llvm does not apply it to values.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:48
+
+if (isNonConstReferenceType(ParType)) {
+  return true;

You can ellide the braces.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:95
+  // if the location is invalid forget it
+  if (!MatchedDecl->getLocation().isValid())
+return;

Please bail on `isMacroID()` as well, as touching macros can have unpleasant 
side effects and is usually avoided in clang-tidy.



Comment at: clang-tidy/modernize/UseNodiscardCheck.cpp:104
+  // ignored
+
+  SourceLocation retLoc = MatchedDecl->getInnerLocStart();

you can remove that empty line.



Comment at: clang-tidy/modernize/UseNodiscardCheck.h:19
+
+/// \brief Add [[nodiscard]] to non void const member functions with no
+/// arguments or pass by value or pass by const reference arguments

Please surround code-snippets in comments with quotation. As iam bad with 
english with a grain of salt: `non-void` and `const-member-functions`? I feel 
that there need to be hyphens somewhere.



Comment at: docs/ReleaseNotes.rst:174
+  Adds ``[[nodiscard]]`` attributes (introduced in C++17) to member functions 
+  to highlight at compile time where the return value of a function should not
+  be ignored.

I feel `where` sounds a bit weird here. Maybe `which return values should not 
be ignored`?



Comment at: docs/clang-tidy/checks/list.rst:15
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append

please remove the spurious changes here.



Comment at: docs/clang-tidy/checks/modernize-use-nodiscard.rst:38
+Specifies a macro to use instead of ``[[nodiscard]]``. This is useful when 
+maintaining source code that needs to compile with a pre-C++17 compiler.
+

Can you please point the users to 
https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
 to show the `__attribute__` syntax that is supported for non-c++17.



Comment at: test/clang-tidy/modernize-use-nodiscard.cpp:9
+
+class Foo
+{

Please add tests, were macros generate the function decls and ensure these are 
untouched.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55433/new/

https://reviews.llvm.org/D55433



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

[PATCH] D55510: [ExprConstant] Improve memchr/memcmp for type mismatch and multibyte element types

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: lib/AST/ExprConstant.cpp:6151
+Info.Ctx.getBaseElementType(getType(Result.getLValueBase()));
+const bool IsRawByte = BuiltinOp == Builtin::BImemchr ||
+   BuiltinOp == Builtin::BI__builtin_memchr;

Drop the top-level `const`.



Comment at: lib/AST/ExprConstant.cpp:8448
+
+const bool IsRawByte = BuiltinOp == Builtin::BImemcmp ||
+   BuiltinOp == Builtin::BI__builtin_memcmp;

Drop top-level `const`. (Same suggestion anywhere the type is local to a 
function and is not a pointer or reference type.)



Comment at: lib/AST/ExprConstant.cpp:8461
+  }
+  const auto CharTy1Width = Info.Ctx.getTypeSize(CharTy1);
+  // Give up on comparing between elements with disparate widths.

Don't use `auto` here as the type is not spelled out in the initialization.



Comment at: lib/AST/ExprConstant.cpp:8465
+return false;
+  const auto LengthPerElement = CharTy1Width / Info.Ctx.getCharWidth();
+  assert(MaxLength);

Drop `const` and use a non-deduced type.



Comment at: lib/AST/ExprConstant.cpp:8466
+  const auto LengthPerElement = CharTy1Width / Info.Ctx.getCharWidth();
+  assert(MaxLength);
+  for (;;) {

Can you add a string literal to this assert explaining what's gone wrong if it 
triggers?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55510/new/

https://reviews.llvm.org/D55510



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

[PATCH] D55484: ComputeLineNumbers: delete SSE2 vectorization

2018-12-10 Thread Fangrui Song via Phabricator via cfe-commits

MaskRay added a comment.

In D55484#1324983 , @bkramer wrote:

> The performance difference on preprocessing huge files was tiny back then, 
> doesn't surprise me that it disappeared. What did you test this on?


I tested it on

  cat lib/Sema/*.cpp lib/CodeGen/*.cpp > /tmp/all.cpp
  perf stat -r 10 clang -E /tmp/all.cpp [-I extracted from build.ninja]

and `/tmp/all.cpp` (13M) repeated 3 and 9 times.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55484/new/

https://reviews.llvm.org/D55484



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

Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on Windows

2018-12-10 Thread Aaron Ballman via cfe-commits

On Mon, Dec 10, 2018 at 12:30 PM Stella Stamenova  wrote:
>
> Our tests run on drive E:\ (not C:\) which is why we saw this test failing. 
> After this change, the test can now run successfully for us because the 
> temporary files are created and checked for on the C:\ drive. Before this 
> change, the temporary files were created on the E:\ drive and checked for on 
> the C:\ drive.
>
> Are you saying that you have a drive C:\ and the test is failing anyway? Or 
> do you not even have a drive C:\?

I have two drives, C:\ and D:\. When I run this unit test (from my D
drive), I now get:

[ RUN  ] FileManagerTest.getVirtualFileFillsRealPathName
D:\llvm\tools\clang\unittests\Basic\FileManagerTest.cpp(371): error:
Expected: file->tryGetRealPathName()
  Which is: { 'd' (100, 0x64), ':' (58, 0x3A), '/' (47, 0x2F), 't'
(116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
(116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) }
To be equal to: ExpectedResult
  Which is: { 'C' (67, 0x43), ':' (58, 0x3A), '/' (47, 0x2F), 't'
(116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
(116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) }
[  FAILED  ] FileManagerTest.getVirtualFileFillsRealPathName (2 ms)

I do not have a C:\temp or a D:\temp drive. If I move the test
executable onto my C:\ drive, I get a different failure instead:

[ RUN  ] FileManagerTest.getVirtualFileFillsRealPathName
D:\llvm\tools\clang\unittests\Basic\FileManagerTest.cpp(371): error:
Expected: file->tryGetRealPathName()
  Which is: { 'c' (99, 0x63), ':' (58, 0x3A), '/' (47, 0x2F), 't'
(116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
(116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) }
To be equal to: ExpectedResult
  Which is: { 'C' (67, 0x43), ':' (58, 0x3A), '/' (47, 0x2F), 't'
(116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
(116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) }
[  FAILED  ] FileManagerTest.getVirtualFileFillsRealPathName (2 ms)

~Aaron

>
> Thanks,
> -Stella
>
> -Original Message-
> From: Aaron Ballman 
> Sent: Monday, December 10, 2018 6:55 AM
> To: Stella Stamenova 
> Cc: cfe-commits 
> Subject: Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on 
> Windows
>
> On Fri, Dec 7, 2018 at 6:52 PM Stella Stamenova via cfe-commits 
>  wrote:
> >
> > Author: stella.stamenova
> > Date: Fri Dec  7 15:50:05 2018
> > New Revision: 348665
> >
> > URL:
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.
> > org%2Fviewvc%2Fllvm-project%3Frev%3D348665%26view%3Drev&data=02%7C
> > 01%7Cstilis%40microsoft.com%7Cc4e90b6970994d2b15b608d65eaf7ff3%7C72f98
> > 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C636800505193880574&sdata=vv0
> > qe64wy0oqci7RAlCXHxmjREevqS3s%2ByNz83tqQIw%3D&reserved=0
> > Log:
> > [tests] Fix the FileManagerTest getVirtualFile test on Windows
> >
> > Summary: The test passes on Windows only when it is executed on the C: 
> > drive. If the build and tests run on a different drive, the test is 
> > currently failing.
> >
> > Reviewers: kadircet, asmith
> >
> > Subscribers: cfe-commits
> >
> > Differential Revision:
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Frevi
> > ews.llvm.org%2FD55451&data=02%7C01%7Cstilis%40microsoft.com%7Cc4e9
> > 0b6970994d2b15b608d65eaf7ff3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C
> > 0%7C636800505193880574&sdata=0dFS4XR2o85FyI0XHr9Eh4pX%2BbdC2CPhXdd
> > X3lDYaao%3D&reserved=0
> >
> > Modified:
> > cfe/trunk/unittests/Basic/FileManagerTest.cpp
> >
> > Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> > URL:
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.
> > org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Funittests%2FBasic%2FFileMa
> > nagerTest.cpp%3Frev%3D348665%26r1%3D348664%26r2%3D348665%26view%3Ddiff
> > &data=02%7C01%7Cstilis%40microsoft.com%7Cc4e90b6970994d2b15b608d65
> > eaf7ff3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63680050519388057
> > 4&sdata=UkdKXFFYeXogiCIJ8%2B41qUFO2qON9seRUVYziL2%2B9Yg%3D&res
> > erved=0
> > ==
> > 
> > --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
> > +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Fri Dec  7 15:50:05
> > +++ 2018
> > @@ -351,22 +351,34 @@ TEST_F(FileManagerTest, makeAbsoluteUses
> >
> >  // getVirtualFile should always fill the real path.
> >  TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) {
> > +  SmallString<64> CustomWorkingDir;
> > +#ifdef _WIN32
> > +  CustomWorkingDir = "C:/";
> > +#else
> > +  CustomWorkingDir = "/";
> > +#endif
>
> Unfortunately, this is still an issue -- you cannot assume that C:\ is the 
> correct drive letter on Windows. For instance, this unit test fails for me 
> because it turns out to be D:\ on my system.
>
> ~Aaron
>
> > +
> > +  auto FS = IntrusiveRefCntPtr(
> > +  new l

r348777 - ComputeLineNumbers: delete SSE2 vectorization

2018-12-10 Thread Fangrui Song via cfe-commits

Author: maskray
Date: Mon Dec 10 10:10:35 2018
New Revision: 348777

URL: http://llvm.org/viewvc/llvm-project?rev=348777&view=rev
Log:
ComputeLineNumbers: delete SSE2 vectorization

Summary:
SSE2 vectorization was added in 2012, but it is 2018 now and I can't
observe any performance boost (testing clang -E [all Sema/* CodeGen/* with 
proper -I options]) with the existing _mm_movemask_epi8+countTrailingZeros or 
the following SSE4.2 (compiling with -msse4.2):

  __m128i C = _mm_setr_epi8('\r','\n',0,0,0,0,0,0,0,0,0,0,0,0,0,0);
  _mm_cmpestri(C, 2, Chunk, 16, _SIDD_UBYTE_OPS | _SIDD_CMP_EQUAL_ANY | 
_SIDD_POSITIVE_POLARITY | _SIDD_LEAST_SIGNIFICANT)

Delete the vectorization to simplify the code.

Also simplify the code a bit and don't check the line ending sequence \n\r

Reviewers: bkramer, #clang

Reviewed By: bkramer

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D55484

Modified:
cfe/trunk/lib/Basic/SourceManager.cpp

Modified: cfe/trunk/lib/Basic/SourceManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/SourceManager.cpp?rev=348777&r1=348776&r2=348777&view=diff
==
--- cfe/trunk/lib/Basic/SourceManager.cpp (original)
+++ cfe/trunk/lib/Basic/SourceManager.cpp Mon Dec 10 10:10:35 2018
@@ -1216,65 +1216,22 @@ static void ComputeLineNumbers(Diagnosti
 
   const unsigned char *Buf = (const unsigned char *)Buffer->getBufferStart();
   const unsigned char *End = (const unsigned char *)Buffer->getBufferEnd();
-  unsigned Offs = 0;
+  unsigned I = 0;
   while (true) {
 // Skip over the contents of the line.
-const unsigned char *NextBuf = (const unsigned char *)Buf;
+while (Buf[I] != '\n' && Buf[I] != '\r' && Buf[I] != '\0')
+  ++I;
 
-#ifdef __SSE2__
-// Try to skip to the next newline using SSE instructions. This is very
-// performance sensitive for programs with lots of diagnostics and in -E
-// mode.
-__m128i CRs = _mm_set1_epi8('\r');
-__m128i LFs = _mm_set1_epi8('\n');
-
-// First fix up the alignment to 16 bytes.
-while (((uintptr_t)NextBuf & 0xF) != 0) {
-  if (*NextBuf == '\n' || *NextBuf == '\r' || *NextBuf == '\0')
-goto FoundSpecialChar;
-  ++NextBuf;
-}
-
-// Scan 16 byte chunks for '\r' and '\n'. Ignore '\0'.
-while (NextBuf+16 <= End) {
-  const __m128i Chunk = *(const __m128i*)NextBuf;
-  __m128i Cmp = _mm_or_si128(_mm_cmpeq_epi8(Chunk, CRs),
- _mm_cmpeq_epi8(Chunk, LFs));
-  unsigned Mask = _mm_movemask_epi8(Cmp);
-
-  // If we found a newline, adjust the pointer and jump to the handling 
code.
-  if (Mask != 0) {
-NextBuf += llvm::countTrailingZeros(Mask);
-goto FoundSpecialChar;
-  }
-  NextBuf += 16;
-}
-#endif
-
-while (*NextBuf != '\n' && *NextBuf != '\r' && *NextBuf != '\0')
-  ++NextBuf;
-
-#ifdef __SSE2__
-FoundSpecialChar:
-#endif
-Offs += NextBuf-Buf;
-Buf = NextBuf;
-
-if (Buf[0] == '\n' || Buf[0] == '\r') {
-  // If this is \n\r or \r\n, skip both characters.
-  if ((Buf[1] == '\n' || Buf[1] == '\r') && Buf[0] != Buf[1]) {
-++Offs;
-++Buf;
-  }
-  ++Offs;
-  ++Buf;
-  LineOffsets.push_back(Offs);
+if (Buf[I] == '\n' || Buf[I] == '\r') {
+  // If this is \r\n, skip both characters.
+  if (Buf[I] == '\r' && Buf[I+1] == '\n')
+++I;
+  ++I;
+  LineOffsets.push_back(I);
 } else {
-  // Otherwise, this is a null.  If end of file, exit.
-  if (Buf == End) break;
-  // Otherwise, skip the null.
-  ++Offs;
-  ++Buf;
+  // Otherwise, this is a NUL. If end of file, exit.
+  if (Buf+I == End) break;
+  ++I;
 }
   }
 


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

[PATCH] D55484: ComputeLineNumbers: delete SSE2 vectorization

2018-12-10 Thread Fangrui Song via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348777: ComputeLineNumbers: delete SSE2 vectorization 
(authored by MaskRay, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55484/new/

https://reviews.llvm.org/D55484

Files:
  cfe/trunk/lib/Basic/SourceManager.cpp


Index: cfe/trunk/lib/Basic/SourceManager.cpp
===
--- cfe/trunk/lib/Basic/SourceManager.cpp
+++ cfe/trunk/lib/Basic/SourceManager.cpp
@@ -1216,65 +1216,22 @@
 
   const unsigned char *Buf = (const unsigned char *)Buffer->getBufferStart();
   const unsigned char *End = (const unsigned char *)Buffer->getBufferEnd();
-  unsigned Offs = 0;
+  unsigned I = 0;
   while (true) {
 // Skip over the contents of the line.
-const unsigned char *NextBuf = (const unsigned char *)Buf;
+while (Buf[I] != '\n' && Buf[I] != '\r' && Buf[I] != '\0')
+  ++I;
 
-#ifdef __SSE2__
-// Try to skip to the next newline using SSE instructions. This is very
-// performance sensitive for programs with lots of diagnostics and in -E
-// mode.
-__m128i CRs = _mm_set1_epi8('\r');
-__m128i LFs = _mm_set1_epi8('\n');
-
-// First fix up the alignment to 16 bytes.
-while (((uintptr_t)NextBuf & 0xF) != 0) {
-  if (*NextBuf == '\n' || *NextBuf == '\r' || *NextBuf == '\0')
-goto FoundSpecialChar;
-  ++NextBuf;
-}
-
-// Scan 16 byte chunks for '\r' and '\n'. Ignore '\0'.
-while (NextBuf+16 <= End) {
-  const __m128i Chunk = *(const __m128i*)NextBuf;
-  __m128i Cmp = _mm_or_si128(_mm_cmpeq_epi8(Chunk, CRs),
- _mm_cmpeq_epi8(Chunk, LFs));
-  unsigned Mask = _mm_movemask_epi8(Cmp);
-
-  // If we found a newline, adjust the pointer and jump to the handling 
code.
-  if (Mask != 0) {
-NextBuf += llvm::countTrailingZeros(Mask);
-goto FoundSpecialChar;
-  }
-  NextBuf += 16;
-}
-#endif
-
-while (*NextBuf != '\n' && *NextBuf != '\r' && *NextBuf != '\0')
-  ++NextBuf;
-
-#ifdef __SSE2__
-FoundSpecialChar:
-#endif
-Offs += NextBuf-Buf;
-Buf = NextBuf;
-
-if (Buf[0] == '\n' || Buf[0] == '\r') {
-  // If this is \n\r or \r\n, skip both characters.
-  if ((Buf[1] == '\n' || Buf[1] == '\r') && Buf[0] != Buf[1]) {
-++Offs;
-++Buf;
-  }
-  ++Offs;
-  ++Buf;
-  LineOffsets.push_back(Offs);
+if (Buf[I] == '\n' || Buf[I] == '\r') {
+  // If this is \r\n, skip both characters.
+  if (Buf[I] == '\r' && Buf[I+1] == '\n')
+++I;
+  ++I;
+  LineOffsets.push_back(I);
 } else {
-  // Otherwise, this is a null.  If end of file, exit.
-  if (Buf == End) break;
-  // Otherwise, skip the null.
-  ++Offs;
-  ++Buf;
+  // Otherwise, this is a NUL. If end of file, exit.
+  if (Buf+I == End) break;
+  ++I;
 }
   }
 


Index: cfe/trunk/lib/Basic/SourceManager.cpp
===
--- cfe/trunk/lib/Basic/SourceManager.cpp
+++ cfe/trunk/lib/Basic/SourceManager.cpp
@@ -1216,65 +1216,22 @@
 
   const unsigned char *Buf = (const unsigned char *)Buffer->getBufferStart();
   const unsigned char *End = (const unsigned char *)Buffer->getBufferEnd();
-  unsigned Offs = 0;
+  unsigned I = 0;
   while (true) {
 // Skip over the contents of the line.
-const unsigned char *NextBuf = (const unsigned char *)Buf;
+while (Buf[I] != '\n' && Buf[I] != '\r' && Buf[I] != '\0')
+  ++I;
 
-#ifdef __SSE2__
-// Try to skip to the next newline using SSE instructions. This is very
-// performance sensitive for programs with lots of diagnostics and in -E
-// mode.
-__m128i CRs = _mm_set1_epi8('\r');
-__m128i LFs = _mm_set1_epi8('\n');
-
-// First fix up the alignment to 16 bytes.
-while (((uintptr_t)NextBuf & 0xF) != 0) {
-  if (*NextBuf == '\n' || *NextBuf == '\r' || *NextBuf == '\0')
-goto FoundSpecialChar;
-  ++NextBuf;
-}
-
-// Scan 16 byte chunks for '\r' and '\n'. Ignore '\0'.
-while (NextBuf+16 <= End) {
-  const __m128i Chunk = *(const __m128i*)NextBuf;
-  __m128i Cmp = _mm_or_si128(_mm_cmpeq_epi8(Chunk, CRs),
- _mm_cmpeq_epi8(Chunk, LFs));
-  unsigned Mask = _mm_movemask_epi8(Cmp);
-
-  // If we found a newline, adjust the pointer and jump to the handling code.
-  if (Mask != 0) {
-NextBuf += llvm::countTrailingZeros(Mask);
-goto FoundSpecialChar;
-  }
-  NextBuf += 16;
-}
-#endif
-
-while (*NextBuf != '\n' && *NextBuf != '\r' && *NextBuf != '\0')
-  ++NextBuf;
-
-#ifdef __SSE2__
-FoundSpecialChar:
-#endif
-Offs += NextBuf-Buf;
-Buf = NextBuf;
-
-if (Buf[0] == '\n' || Buf[0] == '\r') {
-  // If this is \n\r or \r\n, skip both characters.
-  if ((Buf[

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

In D55508#1325670 , @JonasToth wrote:

> I think this patch is fine, AFAIK these utility scripts are not tested 
> directly but are just adjusted if they dont work as expected :)
>
> Did you test it with a fake new-check? If it does what we expect its fine :)

I tested it with both a couple of new fake checks, but also removed everything 
back to how the docs/ReleaseNotes.rst would be after the release branches, i.e. 
with just "The improvements are..."

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508

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

RE: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on Windows

2018-12-10 Thread Stella Stamenova via cfe-commits

The failure that you are getting when you run on the d:\ drive is what we were 
seeing before this change in our testing. What do you get without this change?

Thanks,
-Stella

-Original Message-
From: Aaron Ballman  
Sent: Monday, December 10, 2018 10:11 AM
To: Stella Stamenova 
Cc: cfe-commits 
Subject: Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on 
Windows

On Mon, Dec 10, 2018 at 12:30 PM Stella Stamenova  wrote:
>
> Our tests run on drive E:\ (not C:\) which is why we saw this test failing. 
> After this change, the test can now run successfully for us because the 
> temporary files are created and checked for on the C:\ drive. Before this 
> change, the temporary files were created on the E:\ drive and checked for on 
> the C:\ drive.
>
> Are you saying that you have a drive C:\ and the test is failing anyway? Or 
> do you not even have a drive C:\?

I have two drives, C:\ and D:\. When I run this unit test (from my D drive), I 
now get:

[ RUN  ] FileManagerTest.getVirtualFileFillsRealPathName
D:\llvm\tools\clang\unittests\Basic\FileManagerTest.cpp(371): error:
Expected: file->tryGetRealPathName()
  Which is: { 'd' (100, 0x64), ':' (58, 0x3A), '/' (47, 0x2F), 't'
(116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
(116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) } To be equal 
to: ExpectedResult
  Which is: { 'C' (67, 0x43), ':' (58, 0x3A), '/' (47, 0x2F), 't'
(116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
(116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) } [  FAILED  ] 
FileManagerTest.getVirtualFileFillsRealPathName (2 ms)

I do not have a C:\temp or a D:\temp drive. If I move the test executable onto 
my C:\ drive, I get a different failure instead:

[ RUN  ] FileManagerTest.getVirtualFileFillsRealPathName
D:\llvm\tools\clang\unittests\Basic\FileManagerTest.cpp(371): error:
Expected: file->tryGetRealPathName()
  Which is: { 'c' (99, 0x63), ':' (58, 0x3A), '/' (47, 0x2F), 't'
(116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
(116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) } To be equal 
to: ExpectedResult
  Which is: { 'C' (67, 0x43), ':' (58, 0x3A), '/' (47, 0x2F), 't'
(116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
(116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) } [  FAILED  ] 
FileManagerTest.getVirtualFileFillsRealPathName (2 ms)

~Aaron

>
> Thanks,
> -Stella
>
> -Original Message-
> From: Aaron Ballman 
> Sent: Monday, December 10, 2018 6:55 AM
> To: Stella Stamenova 
> Cc: cfe-commits 
> Subject: Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile 
> test on Windows
>
> On Fri, Dec 7, 2018 at 6:52 PM Stella Stamenova via cfe-commits 
>  wrote:
> >
> > Author: stella.stamenova
> > Date: Fri Dec  7 15:50:05 2018
> > New Revision: 348665
> >
> > URL:
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.
> > org%2Fviewvc%2Fllvm-project%3Frev%3D348665%26view%3Drev&data=02%
> > 7C
> > 01%7Cstilis%40microsoft.com%7Cc4e90b6970994d2b15b608d65eaf7ff3%7C72f
> > 98
> > 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C636800505193880574&sdata=v
> > v0
> > qe64wy0oqci7RAlCXHxmjREevqS3s%2ByNz83tqQIw%3D&reserved=0
> > Log:
> > [tests] Fix the FileManagerTest getVirtualFile test on Windows
> >
> > Summary: The test passes on Windows only when it is executed on the C: 
> > drive. If the build and tests run on a different drive, the test is 
> > currently failing.
> >
> > Reviewers: kadircet, asmith
> >
> > Subscribers: cfe-commits
> >
> > Differential Revision:
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fre
> > vi
> > ews.llvm.org%2FD55451&data=02%7C01%7Cstilis%40microsoft.com%7Cc4
> > e9 
> > 0b6970994d2b15b608d65eaf7ff3%7C72f988bf86f141af91ab2d7cd011db47%7C1%
> > 7C 
> > 0%7C636800505193880574&sdata=0dFS4XR2o85FyI0XHr9Eh4pX%2BbdC2CPhX
> > dd
> > X3lDYaao%3D&reserved=0
> >
> > Modified:
> > cfe/trunk/unittests/Basic/FileManagerTest.cpp
> >
> > Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> > URL:
> > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.
> > org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Funittests%2FBasic%2FFile
> > Ma 
> > nagerTest.cpp%3Frev%3D348665%26r1%3D348664%26r2%3D348665%26view%3Ddi
> > ff
> > &data=02%7C01%7Cstilis%40microsoft.com%7Cc4e90b6970994d2b15b608d
> > 65
> > eaf7ff3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636800505193880
> > 57 
> > 4&sdata=UkdKXFFYeXogiCIJ8%2B41qUFO2qON9seRUVYziL2%2B9Yg%3D&r
> > es
> > erved=0
> > 
> > ==
> > 
> > --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
> > +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Fri Dec  7 
> > +++ 15:50:05
> > +++ 2018
> > @@ -351,22 +351,34 @@ TEST_F(FileManagerTest, makeAbsoluteUses
> >
> >  // getVirtualFile should always fill the real path.
> >  TEST_

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.

LGTM. Do you have commit access?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508



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

Re: [PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

2018-12-10 Thread David Blaikie via cfe-commits

Would it be worth considering whether -fdebug-compilation-dir and
-fdebug-prefix-map could be unified, perhaps by having a placeholder that
could be used in -fdebug-prefix-map for the current directory?

I guess they're low-maintenance enough that it's probably not, but figured
I'd ask.

On Thu, Dec 6, 2018 at 10:34 AM Nico Weber via Phabricator via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> thakis created this revision.
> thakis added a reviewer: rnk.
>
> The flag -fdebug-compilation-dir is useful to make generated .o files
> independent of the path of the build directory, without making the compile
> command-line dependent on the path of the build directory, like
> -fdebug-prefix-map requires. This change makes it so that the driver can
> forward the flag to -cc1as, like it already can for -cc1. We might want to
> consider making -fdebug-compilation-dir a driver flag in a follow-up.
>
> (Since -fdebug-compilation-dir defaults to PWD, it's already possible to
> get this effect by setting PWD, but explicit compiler flags are better than
> env vars, because e.g. ninja tracks command lines and reruns commands that
> change.)
>
> Somewhat related to PR14625.
>
>
> https://reviews.llvm.org/D55377
>
> Files:
>   lib/Driver/ToolChains/Clang.cpp
>   test/Driver/integrated-as.s
>
>
> Index: test/Driver/integrated-as.s
> ===
> --- test/Driver/integrated-as.s
> +++ test/Driver/integrated-as.s
> @@ -50,3 +50,9 @@
>
>  // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC
> -integrated-as %s 2>&1 | FileCheck --check-prefix=PIC %s
>  // PIC: "-mrelocation-model" "pic"
> +
> +// RUN: %clang -### -target x86_64--- -c -integrated-as %s
> -Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
> +// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
> +
> +// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler
> -fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck
> --check-prefix=XA_DEBUGDIR %s
> +// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
> Index: lib/Driver/ToolChains/Clang.cpp
> ===
> --- lib/Driver/ToolChains/Clang.cpp
> +++ lib/Driver/ToolChains/Clang.cpp
> @@ -2152,6 +2152,9 @@
>}
>CmdArgs.push_back(Value.data());
>TakeNextArg = true;
> +  } else if (Value == "-fdebug-compilation-dir") {
> +CmdArgs.push_back("-fdebug-compilation-dir");
> +TakeNextArg = true;
>} else {
>  D.Diag(diag::err_drv_unsupported_option_argument)
>  << A->getOption().getName() << Value;
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55229: [COFF] Statically link certain runtime library functions

2018-12-10 Thread Mandeep Singh Grang via Phabricator via cfe-commits

mgrang added a comment.

Ping for reviews please.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55229/new/

https://reviews.llvm.org/D55229



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

Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on Windows

2018-12-10 Thread Aaron Ballman via cfe-commits

On Mon, Dec 10, 2018 at 1:18 PM Stella Stamenova  wrote:
>
> The failure that you are getting when you run on the d:\ drive is what we 
> were seeing before this change in our testing. What do you get without this 
> change?

When I reverted the change, I got the same behavior -- so then I
double-checked my build logs and discovered the issue. For some
reason, the unit tests were not rebuilding for me despite having dirty
files. When I un-reverted the changes and manually rebuilt
BasicTests.exe, the test started passing. So I think this was a local
build issue where my implementation got into a confused state. Sorry
for the noise!

~Aaron

>
> Thanks,
> -Stella
>
> -Original Message-
> From: Aaron Ballman 
> Sent: Monday, December 10, 2018 10:11 AM
> To: Stella Stamenova 
> Cc: cfe-commits 
> Subject: Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile test on 
> Windows
>
> On Mon, Dec 10, 2018 at 12:30 PM Stella Stamenova  
> wrote:
> >
> > Our tests run on drive E:\ (not C:\) which is why we saw this test failing. 
> > After this change, the test can now run successfully for us because the 
> > temporary files are created and checked for on the C:\ drive. Before this 
> > change, the temporary files were created on the E:\ drive and checked for 
> > on the C:\ drive.
> >
> > Are you saying that you have a drive C:\ and the test is failing anyway? Or 
> > do you not even have a drive C:\?
>
> I have two drives, C:\ and D:\. When I run this unit test (from my D drive), 
> I now get:
>
> [ RUN  ] FileManagerTest.getVirtualFileFillsRealPathName
> D:\llvm\tools\clang\unittests\Basic\FileManagerTest.cpp(371): error:
> Expected: file->tryGetRealPathName()
>   Which is: { 'd' (100, 0x64), ':' (58, 0x3A), '/' (47, 0x2F), 't'
> (116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
> (116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) } To be equal 
> to: ExpectedResult
>   Which is: { 'C' (67, 0x43), ':' (58, 0x3A), '/' (47, 0x2F), 't'
> (116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
> (116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) } [  FAILED  ] 
> FileManagerTest.getVirtualFileFillsRealPathName (2 ms)
>
> I do not have a C:\temp or a D:\temp drive. If I move the test executable 
> onto my C:\ drive, I get a different failure instead:
>
> [ RUN  ] FileManagerTest.getVirtualFileFillsRealPathName
> D:\llvm\tools\clang\unittests\Basic\FileManagerTest.cpp(371): error:
> Expected: file->tryGetRealPathName()
>   Which is: { 'c' (99, 0x63), ':' (58, 0x3A), '/' (47, 0x2F), 't'
> (116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
> (116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) } To be equal 
> to: ExpectedResult
>   Which is: { 'C' (67, 0x43), ':' (58, 0x3A), '/' (47, 0x2F), 't'
> (116, 0x74), 'm' (109, 0x6D), 'p' (112, 0x70), '\\' (92, 0x5C), 't'
> (116, 0x74), 'e' (101, 0x65), 's' (115, 0x73), 't' (116, 0x74) } [  FAILED  ] 
> FileManagerTest.getVirtualFileFillsRealPathName (2 ms)
>
> ~Aaron
>
> >
> > Thanks,
> > -Stella
> >
> > -Original Message-
> > From: Aaron Ballman 
> > Sent: Monday, December 10, 2018 6:55 AM
> > To: Stella Stamenova 
> > Cc: cfe-commits 
> > Subject: Re: r348665 - [tests] Fix the FileManagerTest getVirtualFile
> > test on Windows
> >
> > On Fri, Dec 7, 2018 at 6:52 PM Stella Stamenova via cfe-commits 
> >  wrote:
> > >
> > > Author: stella.stamenova
> > > Date: Fri Dec  7 15:50:05 2018
> > > New Revision: 348665
> > >
> > > URL:
> > > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.
> > > org%2Fviewvc%2Fllvm-project%3Frev%3D348665%26view%3Drev&data=02%
> > > 7C
> > > 01%7Cstilis%40microsoft.com%7Cc4e90b6970994d2b15b608d65eaf7ff3%7C72f
> > > 98
> > > 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C636800505193880574&sdata=v
> > > v0
> > > qe64wy0oqci7RAlCXHxmjREevqS3s%2ByNz83tqQIw%3D&reserved=0
> > > Log:
> > > [tests] Fix the FileManagerTest getVirtualFile test on Windows
> > >
> > > Summary: The test passes on Windows only when it is executed on the C: 
> > > drive. If the build and tests run on a different drive, the test is 
> > > currently failing.
> > >
> > > Reviewers: kadircet, asmith
> > >
> > > Subscribers: cfe-commits
> > >
> > > Differential Revision:
> > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fre
> > > vi
> > > ews.llvm.org%2FD55451&data=02%7C01%7Cstilis%40microsoft.com%7Cc4
> > > e9
> > > 0b6970994d2b15b608d65eaf7ff3%7C72f988bf86f141af91ab2d7cd011db47%7C1%
> > > 7C
> > > 0%7C636800505193880574&sdata=0dFS4XR2o85FyI0XHr9Eh4pX%2BbdC2CPhX
> > > dd
> > > X3lDYaao%3D&reserved=0
> > >
> > > Modified:
> > > cfe/trunk/unittests/Basic/FileManagerTest.cpp
> > >
> > > Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
> > > URL:
> > > https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fllvm.
> > > org%2Fviewvc%2Fllvm-project%2Fcfe%2Ftrunk%2Funittests%2FBasic%2FFile
> > > Ma
> > > nag

r348786 - Adding tests for -ast-dump; NFC.

2018-12-10 Thread Aaron Ballman via cfe-commits

Author: aaronballman
Date: Mon Dec 10 10:37:47 2018
New Revision: 348786

URL: http://llvm.org/viewvc/llvm-project?rev=348786&view=rev
Log:
Adding tests for -ast-dump; NFC.

This adds tests for expressions in C.

Added:
cfe/trunk/test/AST/ast-dump-expr.c

Added: cfe/trunk/test/AST/ast-dump-expr.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-expr.c?rev=348786&view=auto
==
--- cfe/trunk/test/AST/ast-dump-expr.c (added)
+++ cfe/trunk/test/AST/ast-dump-expr.c Mon Dec 10 10:37:47 2018
@@ -0,0 +1,339 @@
+// RUN: %clang_cc1 -Wno-unused-value -std=gnu11 -ast-dump %s | FileCheck 
-strict-whitespace %s
+
+void Comma(void) {
+  1, 2, 3;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' ','
+  // CHECK-NEXT: BinaryOperator 0x{{[^ ]*}}  'int' ','
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 1
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 2
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 3
+}
+
+void Assignment(int a) {
+  a = 12;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '='
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 12
+
+  a += a;
+  // CHECK: CompoundAssignOperator 0x{{[^ ]*}}  
'int' '+=' ComputeLHSTy='int' ComputeResultTy='int'
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+}
+
+void Conditionals(int a) {
+  a ? 0 : 1;
+  // CHECK: ConditionalOperator 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 0
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 1
+
+  a ?: 0;
+  // CHECK: BinaryConditionalOperator 0x{{[^ ]*}}  
'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: OpaqueValueExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: OpaqueValueExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 0
+}
+
+void BinaryOperators(int a, int b) {
+  // Logical operators
+  a || b;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '||'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'b' 'int'
+
+  a && b;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '&&'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'b' 'int'
+
+  // Bitwise operators
+  a | b;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '|'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'b' 'int'
+
+  a ^ b;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '^'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'b' 'int'
+
+  a & b;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '&'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'b' 'int'
+
+  // Equality operators
+  a == b;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '=='
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'b' 'int'
+
+  a != b;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '!='
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'b' 'int'
+
+  // Relational operators
+  a < b;
+  // CHECK: BinaryOperator 0x{{[^ ]*}}  'int' '<'
+  // CHECK-NEXT: ImplicitCastExpr
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
+  // CHECK-NEXT: Impl

[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType

2018-12-10 Thread Leonard Chan via Phabricator via cfe-commits

leonardchan updated this revision to Diff 177561.
leonardchan marked an inline comment as done.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55447/new/

https://reviews.llvm.org/D55447

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/address_space_attribute.cpp
  clang/tools/libclang/CXType.cpp

Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -128,7 +128,8 @@
   if (TU && !T.isNull()) {
 // Handle attributed types as the original type
 if (auto *ATT = T->getAs()) {
-  if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes)) {
+  if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) &&
+  ATT->getAttrKind() != attr::AddressSpace) {
 return MakeCXType(ATT->getModifiedType(), TU);
   }
 }
Index: clang/test/Sema/address_space_attribute.cpp
===
--- /dev/null
+++ clang/test/Sema/address_space_attribute.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -ast-dump | FileCheck %s
+
+// Veryify the ordering of the address_space attribute still comes before the
+// type whereas other attributes are still printed after.
+
+template 
+void func() {
+  // CHECK: VarDecl {{.*}} x '__attribute__((address_space(1))) int *'
+  __attribute__((address_space(1))) int *x;
+
+  // CHECK: VarDecl {{.*}} a 'int * __attribute__((noderef))'
+  int __attribute__((noderef)) * a;
+
+  __attribute__((address_space(I))) int *y;
+  // CHECK: VarDecl {{.*}} y '__attribute__((address_space(2))) int *'
+}
+
+void func2() {
+  func<2>();
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5740,28 +5740,27 @@
 // Type Attribute Processing
 //===--===//
 
-/// BuildAddressSpaceAttr - Builds a DependentAddressSpaceType if an expression
-/// is uninstantiated. If instantiated it will apply the appropriate address space
-/// to the type. This function allows dependent template variables to be used in
-/// conjunction with the address_space attribute
-QualType Sema::BuildAddressSpaceAttr(QualType &T, Expr *AddrSpace,
- SourceLocation AttrLoc) {
+/// Build an AddressSpace index from a constant expression and diagnose any
+/// errors related to invalid address_spaces. Returns true on successfully
+/// building an AddressSpace index.
+static bool BuildAddressSpaceIndex(Sema &S, LangAS &ASIdx,
+   const Expr *AddrSpace,
+   SourceLocation AttrLoc) {
   if (!AddrSpace->isValueDependent()) {
-
 llvm::APSInt addrSpace(32);
-if (!AddrSpace->isIntegerConstantExpr(addrSpace, Context)) {
-  Diag(AttrLoc, diag::err_attribute_argument_type)
+if (!AddrSpace->isIntegerConstantExpr(addrSpace, S.Context)) {
+  S.Diag(AttrLoc, diag::err_attribute_argument_type)
   << "'address_space'" << AANT_ArgumentIntegerConstant
   << AddrSpace->getSourceRange();
-  return QualType();
+  return false;
 }
 
 // Bounds checking.
 if (addrSpace.isSigned()) {
   if (addrSpace.isNegative()) {
-Diag(AttrLoc, diag::err_attribute_address_space_negative)
+S.Diag(AttrLoc, diag::err_attribute_address_space_negative)
 << AddrSpace->getSourceRange();
-return QualType();
+return false;
   }
   addrSpace.setIsSigned(false);
 }
@@ -5770,14 +5769,28 @@
 max =
 Qualifiers::MaxAddressSpace - (unsigned)LangAS::FirstTargetAddressSpace;
 if (addrSpace > max) {
-  Diag(AttrLoc, diag::err_attribute_address_space_too_high)
+  S.Diag(AttrLoc, diag::err_attribute_address_space_too_high)
   << (unsigned)max.getZExtValue() << AddrSpace->getSourceRange();
-  return QualType();
+  return false;
 }
 
-LangAS ASIdx =
+ASIdx =
 getLangASFromTargetAS(static_cast(addrSpace.getZExtValue()));
+return true;
+  }
 
+  // Default value for DependentAddressSpaceTypes
+  ASIdx = LangAS::Default;
+  return true;
+}
+
+/// BuildAddressSpaceAttr - Builds a DependentAddressSpaceType if an expression
+/// is uninstantiated. If instantiated it will apply the appropriate address
+/// space to the type. This function allows dependent template variables to be
+/// used in conjunction with the address_space attribute
+QualType Sema::BuildAddressSpaceAttr(QualType &T, LangAS ASIdx, Expr *AddrSpace,
+ SourceLocation AttrLoc) {
+  if (!AddrSpace->isValueDependent()) {
 // If this type is already address space qualified with a different
 // address space, reject it.

Re: r348685 - Move diagnostic enums into Basic.

2018-12-10 Thread David Blaikie via cfe-commits

Hey Richard,

Thanks for cleaning up some of the layering here!

I /think/ I vaguely recall having a conversation with Richard Smith about a
different direction to fix the layering of the diagnostics system - but it
was/is more involved. Ah, here, apparently I sent out a WIP patch & must've
got engaged in other things: https://reviews.llvm.org/D41357 - not sure if
this is a better/workable/useful way forward, or whether we should go with
sinking the specific diagnostics into the Diagnostics library as you've
done/started doing here (I don't have much context in my head right now, so
I forget exactly what's involved in fully pushing them all down into
Diagnostics).

Just some thoughts/directions/ideas,
- Dave

On Fri, Dec 7, 2018 at 9:07 PM Richard Trieu via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rtrieu
> Date: Fri Dec  7 21:05:03 2018
> New Revision: 348685
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348685&view=rev
> Log:
> Move diagnostic enums into Basic.
>
> Move enums from */*Diagnostic.h to Basic/Diagnostic*.h.
> Basic/AllDiagnostics.h
> needs all the enums and moving the sources to Basic prevents a
> Basic->*->Basic
> dependency loop.  This also allows each Basic/Diagnostics*Kinds.td to have
> a
> header at Basic/Diagnostic*.h (except for Common).  The old headers are
> kept in place since other packages are still using them.
>
> Added:
> cfe/trunk/include/clang/Basic/DiagnosticAST.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/AST/ASTDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticAnalysis.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/Analysis/AnalysisDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticComment.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/AST/CommentDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticCrossTU.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/CrossTU/CrossTUDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticDriver.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/Driver/DriverDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticFrontend.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/Frontend/FrontendDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticLex.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/Lex/LexDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticParse.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/Parse/ParseDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticRefactoring.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticSema.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/Sema/SemaDiagnostic.h
> cfe/trunk/include/clang/Basic/DiagnosticSerialization.h
>   - copied, changed from r348541,
> cfe/trunk/include/clang/Serialization/SerializationDiagnostic.h
> Modified:
> cfe/trunk/include/clang/AST/ASTDiagnostic.h
> cfe/trunk/include/clang/AST/CommentDiagnostic.h
> cfe/trunk/include/clang/Analysis/AnalysisDiagnostic.h
> cfe/trunk/include/clang/Basic/AllDiagnostics.h
> cfe/trunk/include/clang/CrossTU/CrossTUDiagnostic.h
> cfe/trunk/include/clang/Driver/DriverDiagnostic.h
> cfe/trunk/include/clang/Frontend/FrontendDiagnostic.h
> cfe/trunk/include/clang/Lex/LexDiagnostic.h
> cfe/trunk/include/clang/Parse/ParseDiagnostic.h
> cfe/trunk/include/clang/Sema/SemaDiagnostic.h
> cfe/trunk/include/clang/Serialization/SerializationDiagnostic.h
> cfe/trunk/include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
>
> Modified: cfe/trunk/include/clang/AST/ASTDiagnostic.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTDiagnostic.h?rev=348685&r1=348684&r2=348685&view=diff
>
> ==
> --- cfe/trunk/include/clang/AST/ASTDiagnostic.h (original)
> +++ cfe/trunk/include/clang/AST/ASTDiagnostic.h Fri Dec  7 21:05:03 2018
> @@ -11,19 +11,9 @@
>  #define LLVM_CLANG_AST_ASTDIAGNOSTIC_H
>
>  #include "clang/Basic/Diagnostic.h"
> +#include "clang/Basic/DiagnosticAST.h"
>
>  namespace clang {
> -  namespace diag {
> -enum {
> -#define DIAG(ENUM,FLAGS,DEFAULT_MAPPING,DESC,GROUP,\
> - SFINAE,NOWERROR,SHOWINSYSHEADER,CATEGORY) ENUM,
> -#define ASTSTART
> -#include "clang/Basic/DiagnosticASTKinds.inc"
> -#undef DIAG
> -  NUM_BUILTIN_AST_DIAGNOSTICS
> -};
> -  }  // end namespace diag
> -
>/// DiagnosticsEngine argument formatting function for diagnostics that
>/// involve AST nodes.
>///
>
> Modified: cfe/trunk/include/clang/AST/CommentDiagnostic.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/CommentDiagnostic.h?rev=348685&r1=348684&r2=348685&view=diff
>
> =

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added a comment.

In D55508#1325758 , @JonasToth wrote:

> LGTM. Do you have commit access?


I do not I'm afraid


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508



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

[PATCH] D55525: [Driver] Add support for -fembed-bitcode for assembly file

2018-12-10 Thread Steven Wu via Phabricator via cfe-commits

steven_wu created this revision.
steven_wu added reviewers: compnerd, dexonsmith.
Herald added a subscriber: jkorous.

Handle -fembed-bitcode for assembly inputs. When the input file is
assembly, write a marker as "__LLVM,__asm" section.

Fix llvm.org/pr39659


Repository:
  rC Clang

https://reviews.llvm.org/D55525

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/embed-bitcode.s
  tools/driver/cc1as_main.cpp

Index: tools/driver/cc1as_main.cpp
===
--- tools/driver/cc1as_main.cpp
+++ tools/driver/cc1as_main.cpp
@@ -33,6 +33,7 @@
 #include "llvm/MC/MCParser/MCAsmParser.h"
 #include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetOptions.h"
@@ -132,6 +133,7 @@
   unsigned NoExecStack : 1;
   unsigned FatalWarnings : 1;
   unsigned IncrementalLinkerCompatible : 1;
+  unsigned EmbedBitcode : 1;
 
   /// The name of the relocation model to use.
   std::string RelocationModel;
@@ -153,6 +155,7 @@
 FatalWarnings = 0;
 IncrementalLinkerCompatible = 0;
 DwarfVersion = 0;
+EmbedBitcode = 0;
   }
 
   static bool CreateFromArgs(AssemblerInvocation &Res,
@@ -284,6 +287,16 @@
   Args.hasArg(OPT_mincremental_linker_compatible);
   Opts.SymbolDefs = Args.getAllArgValues(OPT_defsym);
 
+  // EmbedBitcode Option. If -fembed-bitcode is enabled, set the flag.
+  // EmbedBitcode behaves the same for all embed options for assembly files.
+  if (auto *A = Args.getLastArg(OPT_fembed_bitcode_EQ)) {
+Opts.EmbedBitcode = llvm::StringSwitch(A->getValue())
+.Case("all", 1)
+.Case("bitcode", 1)
+.Case("marker", 1)
+.Default(0);
+  }
+
   return Success;
 }
 
@@ -449,6 +462,16 @@
 Str.get()->InitSections(Opts.NoExecStack);
   }
 
+  // When -fembed-bitcode is passed to clang_as, a 1-byte marker
+  // is emitted in __LLVM,__asm section if the object file is MachO format.
+  if (Opts.EmbedBitcode && Ctx.getObjectFileInfo()->getObjectFileType() ==
+   MCObjectFileInfo::IsMachO) {
+MCSection *AsmLabel = Ctx.getMachOSection(
+"__LLVM", "__asm", MachO::S_REGULAR, 4, SectionKind::getReadOnly());
+Str.get()->SwitchSection(AsmLabel);
+Str.get()->EmitZeros(1);
+  }
+
   // Assembly to object compilation should leverage assembly info.
   Str->setUseAssemblerInfoForParsing(true);
 
Index: test/Driver/embed-bitcode.s
===
--- /dev/null
+++ test/Driver/embed-bitcode.s
@@ -0,0 +1,12 @@
+// REQUIRES: arm-registered-target
+
+// RUN: %clang -c -target armv7-apple-ios10 %s -fembed-bitcode -### 2>&1 | FileCheck %s -check-prefix=CHECK-AS
+// RUN: %clang -c -target armv7-apple-ios10 %s -fembed-bitcode-marker -### 2>&1 | FileCheck %s -check-prefix=CHECK-AS-MARKER
+// CHECK-AS: -cc1as
+// CHECK-AS: -fembed-bitcode=all
+// CHECK-AS-MARKER: -fembed-bitcode=marker
+
+// RUN: %clang -c -target armv7-apple-ios10 %s -fembed-bitcode -o %t.o
+// RUN: llvm-readobj -section-headers %t.o | FileCheck --check-prefix=CHECK-SECTION %s
+// CHECK-SECTION: Name: __asm
+// CHECK-SECTION-NEXT: Segment: __LLVM
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2167,6 +2167,11 @@
 CmdArgs.push_back("-target-feature");
 CmdArgs.push_back(MipsTargetFeature);
   }
+
+  // forward -fembed-bitcode to assmebler
+  if (C.getDriver().embedBitcodeEnabled() ||
+  C.getDriver().embedBitcodeMarkerOnly())
+Args.AddLastArg(CmdArgs, options::OPT_fembed_bitcode_EQ);
 }
 
 static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -675,7 +675,7 @@
   Flags<[DriverOption]>;
 
 def fembed_bitcode_EQ : Joined<["-"], "fembed-bitcode=">,
-Group, Flags<[DriverOption, CC1Option]>, MetaVarName<"">,
+Group, Flags<[DriverOption, CC1Option, CC1AsOption]>, MetaVarName<"">,
 HelpText<"Embed LLVM bitcode (option: off, all, bitcode, marker)">;
 def fembed_bitcode : Flag<["-"], "fembed-bitcode">, Group,
   Alias, AliasArgs<["all"]>,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21230: Do not embed all the cc1 options in bitcode commandline

2018-12-10 Thread Steven Wu via Phabricator via cfe-commits

steven_wu abandoned this revision.
steven_wu added a comment.
Herald added subscribers: jkorous, mehdi_amini.

This is upstreamed by Saleem already


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D21230/new/

https://reviews.llvm.org/D21230



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

r348789 - [constexpr][c++2a] Try-catch blocks in constexpr functions

2018-12-10 Thread Bruno Cardoso Lopes via cfe-commits

Author: bruno
Date: Mon Dec 10 11:03:12 2018
New Revision: 348789

URL: http://llvm.org/viewvc/llvm-project?rev=348789&view=rev
Log:
[constexpr][c++2a] Try-catch blocks in constexpr functions

Implement support for try-catch blocks in constexpr functions, as
proposed in http://wg21.link/P1002 and voted in San Diego for c++20.

The idea is that we can still never throw inside constexpr, so the catch
block is never entered. A try-catch block like this:

try { f(); } catch (...) { }

is then morally equivalent to just

{ f(); }

Same idea should apply for function/constructor try blocks.

rdar://problem/45530773

Differential Revision: https://reviews.llvm.org/D55097

Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
cfe/trunk/test/CXX/drs/dr6xx.cpp
cfe/trunk/www/cxx_status.html

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=348789&r1=348788&r2=348789&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Dec 10 11:03:12 
2018
@@ -2357,6 +2357,13 @@ def warn_cxx11_compat_constexpr_body_inv
   "use of this statement in a constexpr %select{function|constructor}0 "
   "is incompatible with C++ standards before C++14">,
   InGroup, DefaultIgnore;
+def ext_constexpr_body_invalid_stmt_cxx2a : ExtWarn<
+  "use of this statement in a constexpr %select{function|constructor}0 "
+  "is a C++2a extension">, InGroup;
+def warn_cxx17_compat_constexpr_body_invalid_stmt : Warning<
+  "use of this statement in a constexpr %select{function|constructor}0 "
+  "is incompatible with C++ standards before C++2a">,
+  InGroup, DefaultIgnore;
 def ext_constexpr_type_definition : ExtWarn<
   "type definition in a constexpr %select{function|constructor}0 "
   "is a C++14 extension">, InGroup;
@@ -2409,6 +2416,16 @@ def note_constexpr_body_previous_return
   "previous return statement is here">;
 def err_constexpr_function_try_block : Error<
   "function try block not allowed in constexpr 
%select{function|constructor}0">;
+
+// c++2a function try blocks in constexpr
+def ext_constexpr_function_try_block_cxx2a : ExtWarn<
+  "function try block in constexpr %select{function|constructor}0 is "
+  "a C++2a extension">, InGroup;
+def warn_cxx17_compat_constexpr_function_try_block : Warning<
+  "function try block in constexpr %select{function|constructor}0 is "
+  "incompatible with C++ standards before C++2a">,
+  InGroup, DefaultIgnore;
+
 def err_constexpr_union_ctor_no_init : Error<
   "constexpr union constructor does not initialize any member">;
 def err_constexpr_ctor_missing_init : Error<

Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=348789&r1=348788&r2=348789&view=diff
==
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Mon Dec 10 11:03:12 2018
@@ -4279,6 +4279,9 @@ static EvalStmtResult EvaluateStmt(StmtR
   case Stmt::CaseStmtClass:
   case Stmt::DefaultStmtClass:
 return EvaluateStmt(Result, Info, cast(S)->getSubStmt(), Case);
+  case Stmt::CXXTryStmtClass:
+// Evaluate try blocks by evaluating all sub statements.
+return EvaluateStmt(Result, Info, cast(S)->getTryBlock(), 
Case);
   }
 }
 

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=348789&r1=348788&r2=348789&view=diff
==
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Dec 10 11:03:12 2018
@@ -1803,7 +1803,7 @@ static void CheckConstexprCtorInitialize
 static bool
 CheckConstexprFunctionStmt(Sema &SemaRef, const FunctionDecl *Dcl, Stmt *S,
SmallVectorImpl &ReturnStmts,
-   SourceLocation &Cxx1yLoc) {
+   SourceLocation &Cxx1yLoc, SourceLocation &Cxx2aLoc) 
{
   // - its function-body shall be [...] a compound-statement that contains only
   switch (S->getStmtClass()) {
   case Stmt::NullStmtClass:
@@ -1840,7 +1840,7 @@ CheckConstexprFunctionStmt(Sema &SemaRef
 CompoundStmt *CompStmt = cast(S);
 for (auto *BodyIt : CompStmt->body()) {
   if (!CheckConstexprFunctionStmt(SemaRef, Dcl, BodyIt, ReturnStmts,
-  Cxx1yLoc))
+  Cxx1yLoc, Cxx2aLoc))
 return false;
 }
 return true;
@@ -1858,11 +1858,11 @@ CheckConstex

[PATCH] D55097: [constexpr][c++2a] Try-catch blocks in constexpr functions

2018-12-10 Thread Bruno Cardoso Lopes via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348789: [constexpr][c++2a] Try-catch blocks in constexpr 
functions (authored by bruno, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55097?vs=177367&id=177564#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55097/new/

https://reviews.llvm.org/D55097

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/AST/ExprConstant.cpp
  cfe/trunk/lib/Sema/SemaDeclCXX.cpp
  cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
  cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  cfe/trunk/test/CXX/drs/dr6xx.cpp
  cfe/trunk/www/cxx_status.html

Index: cfe/trunk/www/cxx_status.html
===
--- cfe/trunk/www/cxx_status.html
+++ cfe/trunk/www/cxx_status.html
@@ -953,13 +953,15 @@
 
   Relaxations of constexpr restrictions
   http://wg21.link/p1064r0";>P1064R0
-  No
+  No
 

 http://wg21.link/p1002r1";>P1002R1
+SVN
   
   
 http://wg21.link/p1327r1";>P1327R1
+No
   
   
 http://wg21.link/p1330r0";>P1330R0
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2357,6 +2357,13 @@
   "use of this statement in a constexpr %select{function|constructor}0 "
   "is incompatible with C++ standards before C++14">,
   InGroup, DefaultIgnore;
+def ext_constexpr_body_invalid_stmt_cxx2a : ExtWarn<
+  "use of this statement in a constexpr %select{function|constructor}0 "
+  "is a C++2a extension">, InGroup;
+def warn_cxx17_compat_constexpr_body_invalid_stmt : Warning<
+  "use of this statement in a constexpr %select{function|constructor}0 "
+  "is incompatible with C++ standards before C++2a">,
+  InGroup, DefaultIgnore;
 def ext_constexpr_type_definition : ExtWarn<
   "type definition in a constexpr %select{function|constructor}0 "
   "is a C++14 extension">, InGroup;
@@ -2409,6 +2416,16 @@
   "previous return statement is here">;
 def err_constexpr_function_try_block : Error<
   "function try block not allowed in constexpr %select{function|constructor}0">;
+
+// c++2a function try blocks in constexpr
+def ext_constexpr_function_try_block_cxx2a : ExtWarn<
+  "function try block in constexpr %select{function|constructor}0 is "
+  "a C++2a extension">, InGroup;
+def warn_cxx17_compat_constexpr_function_try_block : Warning<
+  "function try block in constexpr %select{function|constructor}0 is "
+  "incompatible with C++ standards before C++2a">,
+  InGroup, DefaultIgnore;
+
 def err_constexpr_union_ctor_no_init : Error<
   "constexpr union constructor does not initialize any member">;
 def err_constexpr_ctor_missing_init : Error<
Index: cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -1,5 +1,6 @@
-// RUN: %clang_cc1 -verify -std=c++11 -fcxx-exceptions -Werror=c++1y-extensions %s
-// RUN: %clang_cc1 -verify -std=c++1y -fcxx-exceptions -DCXX1Y %s
+// RUN: %clang_cc1 -verify -std=c++11 -fcxx-exceptions -Werror=c++1y-extensions -Werror=c++2a-extensions %s
+// RUN: %clang_cc1 -verify -std=c++1y -fcxx-exceptions -DCXX1Y -Werror=c++2a-extensions %s
+// RUN: %clang_cc1 -verify -std=c++2a -fcxx-exceptions -DCXX1Y -DCXX2A %s
 
 namespace N {
   typedef char C;
@@ -49,8 +50,14 @@
 // - its function-body shall not be a function-try-block;
 struct U {
   constexpr U()
-try // expected-error {{function try block not allowed in constexpr constructor}}
+try
+#ifndef CXX2A
+  // expected-error@-2 {{function try block in constexpr constructor is a C++2a extension}}
+#endif
 : u() {
+#ifndef CXX1Y
+  // expected-error@-2 {{use of this statement in a constexpr constructor is a C++14 extension}}
+#endif
   } catch (...) {
 throw;
   }
Index: cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
===
--- cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
+++ cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
@@ -1,5 +1,6 @@
-// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu -std=c++11 -Werror=c++1y-extensions %s
-// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu -std=c++1y -DCXX1Y %s
+// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu -std=c++11 -Werror=c++1y-extensions -Werror=c++2a-extensions %s
+// RUN: %clang_cc1 -verify -fcxx-exceptions -triple=x86_64-linux-gnu -std=c++1y -DCXX1Y -Werror=c++2a-extensions %s
+// RUN: %clang_cc1 -verify -fcxx-exceptions -t

[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.

2018-12-10 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added inline comments.



Comment at: libcxx/include/memory:1645
 
-template 
+template 
 _LIBCPP_INLINE_VISIBILITY

Quuxplusone wrote:
> ldionne wrote:
> > Coming at it from a slightly different angle, I would think this is what we 
> > want:
> > 
> > ```
> > template  >   class _RawSourceTp = typename remove_const<_SourceTp>::type,
> >   class _RawDestTp = typename remove_const<_DestTp>::type>
> > _LIBCPP_INLINE_VISIBILITY static typename enable_if<
> >
> > // We can use memcpy instead of a loop with construct if...
> > is_trivially_move_constructible<_DestTp>::value && 
> > // - the Dest is trivially move constructible, and
> > is_same<_RawSourceTp, _RawDestTp>::value &&
> > // - both types are the same modulo constness, and either
> > (__is_default_allocator::value ||  
> > //   + the allocator is the default allocator (and we know `construct` is 
> > just placement-new), or
> >  !__has_construct::value), 
> > //   + the allocator does not provide a custom `construct` method (so we'd 
> > fall back to placement-new)
> > void>::type
> > __construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp* 
> > __end1, _DestTp*& __begin2)
> > {
> > ptrdiff_t _Np = __end1 - __begin1;
> > if (_Np > 0)
> > {
> > _VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np * 
> > sizeof(_DestTp));
> > __begin2 += _Np;
> > }
> > }
> > ```
> > 
> > And then we should have
> > 
> > ```
> > template 
> > struct __is_default_allocator : false_type { };
> > 
> > template 
> > struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type { };
> > ```
> > 
> > Does this make sense?
> > 
> > Also, I'm not sure I understand why we use `const_cast` on the destination 
> > type. It seems like we should instead enforce that it is non-const? But 
> > this is a pre-existing thing in the code, this doesn't affect this review.
> > 
> I agree that it is wrong to express the check in terms of 
> `is_same>`; it should be expressed in terms of 
> a trait which is satisfied by `std::allocator`-for-any-T. @ldionne 
> expressed it in terms of `__is_default_allocator`. I continue to ask that 
> it be expressed in terms of `__has_trivial_construct _SourceTp&>`, where libc++ specializes 
> `__has_trivial_construct, ...>` if need be.
> 
> Orthogonally, the condition `__has_construct _SourceTp const&>` is wrong because it has an extra `const`. It is 
> conceivable — though of course implausible/pathological — for `construct(T*, 
> T&)` to exist and do something different from `construct(T*, const T&)`.
> I continue to ask that it be expressed in terms of 
> `__has_trivial_construct`, where libc++ specializes 
> `__has_trivial_construct, ...>` if need be.

Would you be OK with us applying this fix and then generalizing 
`__is_default_allocator` into `__has_trivial_construct` as a followup? I 
suspect we'll have more discussion around that generalization and I'd like for 
us to fix this bug because I find PR37574 somewhat concerning and I'd like for 
it to be fixed soon (like within a couple of days).

> Orthogonally, the condition `__has_construct _SourceTp const&>` is wrong because it has an extra const. It is conceivable 
> — though of course implausible/pathological — for `construct(T*, T&)` to 
> exist and do something different from `construct(T*, const T&)`.


Good catch. IIUC, `__has_construct` would 
work?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D48342/new/

https://reviews.llvm.org/D48342



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

r348790 - Add an explicit triple to this test to fix failing test bots.

2018-12-10 Thread Aaron Ballman via cfe-commits

Author: aaronballman
Date: Mon Dec 10 11:18:11 2018
New Revision: 348790

URL: http://llvm.org/viewvc/llvm-project?rev=348790&view=rev
Log:
Add an explicit triple to this test to fix failing test bots.

Modified:
cfe/trunk/test/AST/ast-dump-expr.c

Modified: cfe/trunk/test/AST/ast-dump-expr.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-expr.c?rev=348790&r1=348789&r2=348790&view=diff
==
--- cfe/trunk/test/AST/ast-dump-expr.c (original)
+++ cfe/trunk/test/AST/ast-dump-expr.c Mon Dec 10 11:18:11 2018
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -Wno-unused-value -std=gnu11 -ast-dump %s | FileCheck 
-strict-whitespace %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -std=gnu11 
-ast-dump %s | FileCheck -strict-whitespace %s
 
 void Comma(void) {
   1, 2, 3;
@@ -213,15 +213,15 @@ void UnaryOperators(int a, int *b) {
   // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
 
   sizeof a;
-  // CHECK: UnaryExprOrTypeTraitExpr 0x{{[^ ]*}}  
'unsigned long long' sizeof
+  // CHECK: UnaryExprOrTypeTraitExpr 0x{{[^ ]*}}  
'unsigned long' sizeof
   // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'a' 'int'
 
   sizeof(int);
-  // CHECK: UnaryExprOrTypeTraitExpr 0x{{[^ ]*}}  
'unsigned long long' sizeof 'int'
+  // CHECK: UnaryExprOrTypeTraitExpr 0x{{[^ ]*}}  
'unsigned long' sizeof 'int'
 
   _Alignof(int);
   // FIXME: Uses C++ spelling for alignof in C mode.
-  // CHECK: UnaryExprOrTypeTraitExpr 0x{{[^ ]*}}  
'unsigned long long' alignof 'int'
+  // CHECK: UnaryExprOrTypeTraitExpr 0x{{[^ ]*}}  
'unsigned long' alignof 'int'
 }
 
 struct S {
@@ -287,7 +287,7 @@ void PrimaryExpressions(int a) {
   // CHECK: CharacterLiteral 0x{{[^ ]*}}  'int' 97
 
   L'a';
-  // CHECK: CharacterLiteral 0x{{[^ ]*}}  'unsigned short' 
97
+  // CHECK: CharacterLiteral 0x{{[^ ]*}}  'int' 97
 
   "a";
   // ImplicitCastExpr
@@ -295,7 +295,7 @@ void PrimaryExpressions(int a) {
 
   L"a";
   // ImplicitCastExpr
-  // CHECK: StringLiteral 0x{{[^ ]*}}  'unsigned short [2]' lvalue L"a"
+  // CHECK: StringLiteral 0x{{[^ ]*}}  'int [2]' lvalue L"a"
 
   u8"a";
   // ImplicitCastExpr


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

[PATCH] D55500: [Builtins] Implement __builtin_is_constant_evaluated for use in C++2a

2018-12-10 Thread Bruno Cardoso Lopes via Phabricator via cfe-commits

bruno added a comment.

Hi Eric,

Thanks for working on this!




Comment at: include/clang/Basic/Builtins.def:759
+// Random C++ builtins.
+LANGBUILTIN(__builtin_is_constant_evaluated, "b", "ncu", CXX_LANG)
+

Name bikeshedding : perhaps the builtin name could be detached from the std:: 
name? Suggestion: `__builtin_in_constant_evaluation_context`



Comment at: lib/AST/ExprConstant.cpp:8207
 
+  case Builtin::BI__builtin_is_constant_evaluated:
+return Success(Info.InConstantContext, E);

Do we need compat and extension warnings for the use of this builtin before 
c++2a? I assume people will play with the builtin before the library facility 
is there. OTOH, since this will be mainly exposed as a library thing, whatever 
check for c++ version is done at the library level should be enough?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55500/new/

https://reviews.llvm.org/D55500



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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

2018-12-10 Thread Leonard Chan via Phabricator via cfe-commits

leonardchan added a comment.

In D53738#1320936 , @rjmccall wrote:

> Okay, thanks, that makes sense to me.
>
> I'll ask around to find a way to contact the C committee.

@rjmccall Any updates on this? If we aren't able to find a way to contact 
anyone affiliated with publishing the spec, we could at least double check 
against avr-gcc for consistency regarding semantics.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53738/new/

https://reviews.llvm.org/D53738

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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth added a comment.

In D55508#1325834 , @MyDeveloperDay 
wrote:

> In D55508#1325758 , @JonasToth wrote:
>
> > LGTM. Do you have commit access?
>
>
> I do not I'm afraid


I will commit for you.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Jonas Toth via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348793: [clang-tidy]  insert release notes for new checkers 
alphabetically (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55508?vs=177538&id=177578#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508

Files:
  clang-tools-extra/trunk/clang-tidy/add_new_check.py


Index: clang-tools-extra/trunk/clang-tidy/add_new_check.py
===
--- clang-tools-extra/trunk/clang-tidy/add_new_check.py
+++ clang-tools-extra/trunk/clang-tidy/add_new_check.py
@@ -200,23 +200,47 @@
   with open(filename, 'r') as f:
 lines = f.readlines()
 
+  lineMatcher = re.compile('Improvements to clang-tidy')
+  nextSectionMatcher = re.compile('Improvements to include-fixer')
+  checkerMatcher = re.compile('- New :doc:`(.*)')
+
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
 note_added = False
 header_found = False
+next_header_found = False
+add_note_here = False
 
 for line in lines:
   if not note_added:
-match = re.search('Improvements to clang-tidy', line)
+match = lineMatcher.match(line)
+match_next = nextSectionMatcher.match(line)
+match_checker = checkerMatcher.match(line)
+if match_checker:
+  last_checker = match_checker.group(1)
+  if last_checker > check_name_dashes:
+add_note_here = True
+
+if match_next:
+  next_header_found = True
+  add_note_here = True
+
 if match:
   header_found = True
-elif header_found:
+  f.write(line)
+  continue
+
+if line.startswith(''):
+  f.write(line)
+  continue
+
+if header_found and add_note_here:
   if not line.startswith(''):
-f.write("""
-- New :doc:`%s
+f.write("""- New :doc:`%s
   ` check.
 
   FIXME: add release notes.
+
 """ % (check_name_dashes, check_name_dashes))
 note_added = True
 


Index: clang-tools-extra/trunk/clang-tidy/add_new_check.py
===
--- clang-tools-extra/trunk/clang-tidy/add_new_check.py
+++ clang-tools-extra/trunk/clang-tidy/add_new_check.py
@@ -200,23 +200,47 @@
   with open(filename, 'r') as f:
 lines = f.readlines()
 
+  lineMatcher = re.compile('Improvements to clang-tidy')
+  nextSectionMatcher = re.compile('Improvements to include-fixer')
+  checkerMatcher = re.compile('- New :doc:`(.*)')
+
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
 note_added = False
 header_found = False
+next_header_found = False
+add_note_here = False
 
 for line in lines:
   if not note_added:
-match = re.search('Improvements to clang-tidy', line)
+match = lineMatcher.match(line)
+match_next = nextSectionMatcher.match(line)
+match_checker = checkerMatcher.match(line)
+if match_checker:
+  last_checker = match_checker.group(1)
+  if last_checker > check_name_dashes:
+add_note_here = True
+
+if match_next:
+  next_header_found = True
+  add_note_here = True
+
 if match:
   header_found = True
-elif header_found:
+  f.write(line)
+  continue
+
+if line.startswith(''):
+  f.write(line)
+  continue
+
+if header_found and add_note_here:
   if not line.startswith(''):
-f.write("""
-- New :doc:`%s
+f.write("""- New :doc:`%s
   ` check.
 
   FIXME: add release notes.
+
 """ % (check_name_dashes, check_name_dashes))
 note_added = True
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r348793 - [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Jonas Toth via cfe-commits

Author: jonastoth
Date: Mon Dec 10 11:41:53 2018
New Revision: 348793

URL: http://llvm.org/viewvc/llvm-project?rev=348793&view=rev
Log:
[clang-tidy]  insert release notes for new checkers alphabetically

Summary:
Almost all code review comments on new checkers {D55433} {D48866} {D54349} seem 
to ask for the release notes to be added alphabetically, plus I've seen commits 
by @Eugene.Zelenko reordering the lists

Make add_new_check.py add those release notes alphabetically based on checker 
name

If include-fixer section is seen add it at the end

Minor change in the message format to prevent double newlines added before the 
checker.

Do the tools themselves have unit tests? (sorry new to this game)

- Tested adding new checker at the beginning
- Tested on adding new checker in the middle
- Tested on empty ReleasesNotes.rst (as we would see after RC)

Patch by MyDeveloperDay.

Reviewers: alexfh, JonasToth, curdeius, aaron.ballman, benhamilton, hokein

Reviewed By: JonasToth

Subscribers: cfe-commits, xazax.hun, Eugene.Zelenko

Tags: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D55508

Modified:
clang-tools-extra/trunk/clang-tidy/add_new_check.py

Modified: clang-tools-extra/trunk/clang-tidy/add_new_check.py
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/add_new_check.py?rev=348793&r1=348792&r2=348793&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/add_new_check.py (original)
+++ clang-tools-extra/trunk/clang-tidy/add_new_check.py Mon Dec 10 11:41:53 2018
@@ -200,23 +200,47 @@ def add_release_notes(module_path, modul
   with open(filename, 'r') as f:
 lines = f.readlines()
 
+  lineMatcher = re.compile('Improvements to clang-tidy')
+  nextSectionMatcher = re.compile('Improvements to include-fixer')
+  checkerMatcher = re.compile('- New :doc:`(.*)')
+
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
 note_added = False
 header_found = False
+next_header_found = False
+add_note_here = False
 
 for line in lines:
   if not note_added:
-match = re.search('Improvements to clang-tidy', line)
+match = lineMatcher.match(line)
+match_next = nextSectionMatcher.match(line)
+match_checker = checkerMatcher.match(line)
+if match_checker:
+  last_checker = match_checker.group(1)
+  if last_checker > check_name_dashes:
+add_note_here = True
+
+if match_next:
+  next_header_found = True
+  add_note_here = True
+
 if match:
   header_found = True
-elif header_found:
+  f.write(line)
+  continue
+
+if line.startswith(''):
+  f.write(line)
+  continue
+
+if header_found and add_note_here:
   if not line.startswith(''):
-f.write("""
-- New :doc:`%s
+f.write("""- New :doc:`%s
   ` check.
 
   FIXME: add release notes.
+
 """ % (check_name_dashes, check_name_dashes))
 note_added = True
 


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

[PATCH] D55488: Add utility for dumping a label with child nodes

2018-12-10 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: include/clang/AST/ASTDumperUtils.h:115
+  template 
+  void addChild(const std::string &label, Fn doAddChild) {
+if (label.empty())

label -> Label
doAddChild -> DoAddChild



Comment at: include/clang/AST/ASTDumperUtils.h:117
+if (label.empty())
+  return addChild(doAddChild);
+addChild([=] {

While correct, this is a bit too clever -- can you switch to using a more 
idiomatic return statement?



Comment at: lib/AST/ASTDumper.cpp:66
+template 
+void dumpChild(const std::string &label, Fn doDumpChild) {
+  TreeStructure.addChild(label, doDumpChild);

Label, DoDumpChild



Comment at: lib/AST/ASTDumper.cpp:89
 void dumpDecl(const Decl *D);
-void dumpStmt(const Stmt *S);
+void dumpStmt(const Stmt *S, const std::string &label = {});
 

Label

Rather than using `{}`, how about `""` (same behavior, but looks more 
idiomatic).

Why `std::string` instead of `StringRef`? I expect this will be called mostly 
with string literals, which saves an allocation. The other labels are using 
`const char *`, which would be another reasonable option. Whatever we go with, 
it'd be nice to make the label types agree across the calls.



Comment at: lib/AST/ASTDumper.cpp:1692
 
-void ASTDumper::dumpStmt(const Stmt *S) {
-  dumpChild([=] {
+void ASTDumper::dumpStmt(const Stmt *S, const std::string &label) {
+  dumpChild(label, [=] {

label -> Label


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55488/new/

https://reviews.llvm.org/D55488



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added a comment.

By the word, will be good idea to have script which check alphabetical order 
and use it during build. Sometimes alphabetical order may be violated after 
merge with trunk.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508



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

[PATCH] D55508: [clang-tidy] insert release notes for new checkers alphabetically

2018-12-10 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth added a comment.

In D55508#1325960 , @Eugene.Zelenko 
wrote:

> By the word, will be good idea to have script which check alphabetical order 
> and use it during build. Sometimes alphabetical order may be violated after 
> merge with trunk.

I think that should be in the `validate-check.py` proposed in the other patch. 
If we just add it to the normal testing/documentation generation it would be 
best. I think it is like a unit-test in spirit.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55508/new/

https://reviews.llvm.org/D55508

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

[PATCH] D54489: Implement -frecord-command-line (-frecord-gcc-switches)

2018-12-10 Thread Scott Linder via Phabricator via cfe-commits

scott.linder added a comment.

There may be changes to some details in the LLVM patch; once they are finalized 
I will update the Clang patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54489/new/

https://reviews.llvm.org/D54489



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

[PATCH] D55527: Normalize GlobalDecls when used with CPUDispatch

2018-12-10 Thread Erich Keane via Phabricator via cfe-commits

erichkeane created this revision.
erichkeane added reviewers: rsmith, aaron.ballman.

Previously, CPUDispatch functionality did some wacky things with
GlobalDecls, which resulted in multiple GlobalDecls having the same
mangled name. This patch corrects this in a few ways:

First, it Moves the emission of CPUDispatch resolvers to the end of the
translation unit.  This is akin to how 'target' multiversioning works,
and permits the resolver emission mechanism to see all possible
FunctionDecls in the TU.

Second, it changes the meaning of the GlobalDecl MultiVersionIndex to
MultiVersionOrdinal.  In the case of CPUSpecific, '0' now means "A call
to a non-present CPUDispatch function".  Other ordinals are the order of
the CPU Name in the attribute source.  In the case of CPUDispatch, '0'
is the resolver function, and other ordinals represent the non-present
CPU-Specific version.

Because of this, the 1:1 relationship between a GlobalDecl and a
Mangled Name is restored. Additionally, it results in functions only
being emitted when necessary, rather than use a variety of hackery to
emit in cases we presume they would be used.


https://reviews.llvm.org/D55527

Files:
  include/clang/AST/GlobalDecl.h
  include/clang/Basic/Attr.td
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  test/CodeGen/attr-cpuspecific.c

Index: test/CodeGen/attr-cpuspecific.c
===
--- test/CodeGen/attr-cpuspecific.c
+++ test/CodeGen/attr-cpuspecific.c
@@ -29,21 +29,7 @@
 
 ATTR(cpu_dispatch(ivybridge, knl))
 void TwoVersions(void);
-// LINUX: define void ()* @TwoVersions.resolver()
-// LINUX: call void @__cpu_indicator_init
-// LINUX: ret void ()* @TwoVersions.Z
-// LINUX: ret void ()* @TwoVersions.S
-// LINUX: call void @llvm.trap
-// LINUX: unreachable
-
-// WINDOWS: define dso_local void @TwoVersions()
-// WINDOWS: call void @__cpu_indicator_init()
-// WINDOWS: call void @TwoVersions.Z()
-// WINDOWS-NEXT: ret void
-// WINDOWS: call void @TwoVersions.S()
-// WINDOWS-NEXT: ret void
-// WINDOWS: call void @llvm.trap
-// WINDOWS: unreachable
+// Resolvers are emitted at the end, so the check lines are at the bottom.
 
 ATTR(cpu_specific(ivybridge))
 void TwoVersions(void){}
@@ -82,6 +68,59 @@
 // has an extra config to emit!
 ATTR(cpu_dispatch(ivybridge, knl, atom))
 void TwoVersionsSameAttr(void);
+// Resolvers are emitted at the end, so the check lines are at the bottom.
+
+ATTR(cpu_dispatch(atom, ivybridge, knl))
+void ThreeVersionsSameAttr(void){}
+// Resolvers are emitted at the end, so the check lines are at the bottom.
+
+// No Cpu Specific options.
+ATTR(cpu_dispatch(atom, ivybridge, knl))
+void NoSpecifics(void);
+// Resolvers are emitted at the end, so the check lines are at the bottom.
+
+ATTR(cpu_dispatch(atom, generic, ivybridge, knl))
+void HasGeneric(void);
+// Resolvers are emitted at the end, so the check lines are at the bottom.
+
+ATTR(cpu_dispatch(atom, generic, ivybridge, knl))
+void HasParams(int i, double d);
+// Resolvers are emitted at the end, so the check lines are at the bottom.
+
+ATTR(cpu_dispatch(atom, generic, ivybridge, knl))
+int HasParamsAndReturn(int i, double d);
+// Resolvers are emitted at the end, so the check lines are at the bottom.
+
+ATTR(cpu_dispatch(atom, generic, pentium))
+int GenericAndPentium(int i, double d);
+// Resolvers are emitted at the end, so the check lines are at the bottom.
+
+ATTR(cpu_dispatch(atom, pentium))
+int DispatchFirst(void);
+// Resolvers are emitted at the end, so the check lines are at the bottom.
+
+ATTR(cpu_specific(atom))
+int DispatchFirst(void) {return 0;}
+ATTR(cpu_specific(pentium))
+int DispatchFirst(void) {return 1;}
+// Resolver emit causes these to be emited, so they happen later.
+
+// LINUX: define void ()* @TwoVersions.resolver()
+// LINUX: call void @__cpu_indicator_init
+// LINUX: ret void ()* @TwoVersions.Z
+// LINUX: ret void ()* @TwoVersions.S
+// LINUX: call void @llvm.trap
+// LINUX: unreachable
+
+// WINDOWS: define dso_local void @TwoVersions()
+// WINDOWS: call void @__cpu_indicator_init()
+// WINDOWS: call void @TwoVersions.Z()
+// WINDOWS-NEXT: ret void
+// WINDOWS: call void @TwoVersions.S()
+// WINDOWS-NEXT: ret void
+// WINDOWS: call void @llvm.trap
+// WINDOWS: unreachable
+
 // LINUX: define void ()* @TwoVersionsSameAttr.resolver()
 // LINUX: ret void ()* @TwoVersionsSameAttr.Z
 // LINUX: ret void ()* @TwoVersionsSameAttr.S
@@ -99,8 +138,6 @@
 // WINDOWS: call void @llvm.trap
 // WINDOWS: unreachable
 
-ATTR(cpu_dispatch(atom, ivybridge, knl))
-void ThreeVersionsSameAttr(void){}
 // LINUX: define void ()* @ThreeVersionsSameAttr.resolver()
 // LINUX: call void @__cpu_indicator_init
 // LINUX: ret void ()* @ThreeVersionsSameAttr.Z
@@ -120,9 +157,16 @@
 // WINDOWS: call void @llvm.trap
 // WINDOWS: unreachable
 
-// No Cpu Specific options.
-ATTR(cpu_dispatch(atom, ivybridge, knl))
-void NoSpecifics(void);
+// LINUX: define i32 @DispatchFirst.O
+// LINUX: ret i32 0
+

1 2 >

1 - 100 of 180 matches

Mail list logo