[PATCH] D78902: [Driver] Add output file to properties of Command

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 280796.
sepavloff added a comment.

Rebased patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78902

Files:
  clang/include/clang/Driver/Job.h
  clang/lib/Driver/Job.cpp
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/lib/Driver/ToolChains/AVR.cpp
  clang/lib/Driver/ToolChains/Ananas.cpp
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CloudABI.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CrossWindows.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/DragonFly.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/lib/Driver/ToolChains/Hexagon.cpp
  clang/lib/Driver/ToolChains/InterfaceStubs.cpp
  clang/lib/Driver/ToolChains/MSP430.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/lib/Driver/ToolChains/Minix.cpp
  clang/lib/Driver/ToolChains/Myriad.cpp
  clang/lib/Driver/ToolChains/NaCl.cpp
  clang/lib/Driver/ToolChains/NetBSD.cpp
  clang/lib/Driver/ToolChains/OpenBSD.cpp
  clang/lib/Driver/ToolChains/PS4CPU.cpp
  clang/lib/Driver/ToolChains/RISCVToolchain.cpp
  clang/lib/Driver/ToolChains/Solaris.cpp
  clang/lib/Driver/ToolChains/WebAssembly.cpp
  clang/lib/Driver/ToolChains/XCore.cpp
  clang/unittests/Driver/ToolChainTest.cpp

Index: clang/unittests/Driver/ToolChainTest.cpp
===
--- clang/unittests/Driver/ToolChainTest.cpp
+++ clang/unittests/Driver/ToolChainTest.cpp
@@ -259,4 +259,34 @@
   EXPECT_STREQ(Res.DriverMode, "--driver-mode=cl");
   EXPECT_FALSE(Res.TargetIsValid);
 }
+
+TEST(ToolChainTest, CommandOutput) {
+  IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions();
+
+  IntrusiveRefCntPtr DiagID(new DiagnosticIDs());
+  struct TestDiagnosticConsumer : public DiagnosticConsumer {};
+  DiagnosticsEngine Diags(DiagID, &*DiagOpts, new TestDiagnosticConsumer);
+  IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+
+  Driver CCDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
+  InMemoryFileSystem);
+  CCDriver.setCheckInputsExist(false);
+  std::unique_ptr CC(
+  CCDriver.BuildCompilation({"/home/test/bin/clang", "foo.cpp"}));
+  const JobList &Jobs = CC->getJobs();
+
+  const auto &CmdCompile = Jobs.getJobs().front();
+  const auto &InFile = CmdCompile->getInputFilenames().front();
+  EXPECT_STREQ(InFile, "foo.cpp");
+  auto ObjFile = CmdCompile->getOutputFilenames().front();
+  EXPECT_TRUE(StringRef(ObjFile).endswith(".o"));
+
+  const auto &CmdLink = Jobs.getJobs().back();
+  const auto LinkInFile = CmdLink->getInputFilenames().front();
+  EXPECT_EQ(ObjFile, LinkInFile);
+  auto ExeFile = CmdLink->getOutputFilenames().front();
+  EXPECT_EQ("a.out", ExeFile);
+}
+
 } // end anonymous namespace.
Index: clang/lib/Driver/ToolChains/XCore.cpp
===
--- clang/lib/Driver/ToolChains/XCore.cpp
+++ clang/lib/Driver/ToolChains/XCore.cpp
@@ -53,7 +53,7 @@
 
   const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath("xcc"));
   C.addCommand(std::make_unique(JA, *this, ResponseFileSupport::None(),
- Exec, CmdArgs, Inputs));
+ Exec, CmdArgs, Inputs, Output));
 }
 
 void tools::XCore::Linker::ConstructJob(Compilation &C, const JobAction &JA,
@@ -82,7 +82,7 @@
 
   const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath("xcc"));
   C.addCommand(std::make_unique(JA, *this, ResponseFileSupport::None(),
- Exec, CmdArgs, Inputs));
+ Exec, CmdArgs, Inputs, Output));
 }
 
 /// XCore tool chain
Index: clang/lib/Driver/ToolChains/WebAssembly.cpp
===
--- clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -114,8 +114,9 @@
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
 
-  C.addCommand(std::make_unique(
-  JA, *this, ResponseFileSupport::AtFileCurCP(), Linker, CmdArgs, Inputs));
+  C.addCommand(std::make_unique(JA, *this,
+ ResponseFileSupport::AtFileCurCP(),
+ Linker, CmdArgs, Inputs, Output));
 
   // When optimizing, if wasm-opt is available, run it.
   if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {
@@ -139,7 +140,7 @@
 CmdArgs.push_back(Output.getFilename());
 C.addCommand(std::make_unique(

[PATCH] D84136: [clangd] Fix visitation of ConceptSpecializationExpr in constrained-parameter

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.



Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1843
+  if (const auto *TC = D->getTypeConstraint()) {
+TRY_TO(TraverseStmt(TC->getImmediatelyDeclaredConstraint()));
 TRY_TO(TraverseConceptReference(*TC));

hokein wrote:
> Looks like we may visit some nodes in `ConceptReference` twice:
> -  getImmediatelyDeclaredConstraint returns a `ConceptSpecializationExpr` 
> (most cases?) which is a subclass of `ConceptReference`;
> - `TraverseStmt(ConceptSpecializationExpr*)` will dispatch to 
> `TraverseConceptSpecializationExpr` which invokes `TraverseConceptReference` 
> (see Line 2719);
> 
> 
> It is sad that we don't have enough test coverage, could you write some tests 
> in `clang/unittests/Tooling/RecursiveASTVisitorTests/`?
It is true that there will be two calls to `TraverseConceptReference()`. 
However, they are called on two different `ConceptReference` objects:

  * the call in `TraverseConceptSpecializationExpr` will visit the base 
subobject of the `ConceptSpecializationExpr` (which inherits from 
`ConceptReference`)
  * the call in `TraverseTemplateTypeParmDecl` will visit the base subobject of 
the `TypeConstraint` (which also inherits from `ConceptReference`).

So, I think this is fine -- there are two distinct `ConceptReference` objects 
in the AST, and with this patch we visit both of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84136



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

[PATCH] D82791: [lit] Improve lit's output with default settings and --verbose.

[Varun Gandhi via Phabricator via cfe-commits]

varungandhi-apple added a comment.

Some of the changes probably don't make perfect sense in this revision because 
I initially started out with the whole change being one revision and then split 
things into commits, while trying to minimize the churn between revisions. 
Please also take a look at the follow-up revision D82811 
 and let me know if that's acceptable (less 
churn, less self-contained, things make sense when you look at the two 
together) or if you'd prefer that I make this revision more self-contained 
(that would create more churn in D82811 ).




Comment at: llvm/utils/lit/lit/OutputSettings.py:34
+ONLY_FAILING_COMMAND = CommandOutputStyle("OnlyFailing")
+UP_TO_AND_INCLUDING_FAILING_COMMAND = CommandOutputStyle("UpToAndIncluding")
+

yln wrote:
> I really like the "communicating intent" part of this infrastructure.  
> However, this is a lot of code and `if`s considering we really only have to 
> distinguish two cases.  From your commit message:
> 
> - default (no flags): no script, show only failing line in command output
> - `-v`: full script, adds 'set +x', shows command output
> 
> Am I missing something or could everything be keyed off a single verbose 
> (reading from the code below `showAllOutput` implies verbose) flag.  Do we 
> anticipate other combinations being useful? (In general I would argue for 
> striving for the simplest implementation that gives us what we currently want 
> and not try to anticipate future extensions.)
> 
> Have you considered (or started out with) just using a single verbose flag to 
> base your decisions in the implementation functions?  
Not sure what you mean by two cases, I think there are three cases:

1. Quiet -> Corresponds on `NO_COMMAND`
2. Default (no quiet, no verbose) -> Corresponds to `ONLY_FAILING`.
3. Verbose -> Corresponds to `UP_TO_AND_INCLUDING_FAILING`.

Since there is a 1-1 correspondence, we _could_ compare quiet vs (!quiet && 
!verbose) vs verbose in `make_command_output`. I chose not to do that because I 
figured this makes the intent clearer by distinguishing between user-specified 
options and derived options.

Does that make sense? Would you still prefer that I get rid of this?



Comment at: llvm/utils/lit/lit/TestRunner.py:1503-1508
+Returns a pair of:
+- The index in ``output_str`` pointing to immediately after the preceding
+  newline, i.e. the start of the RUN line, before any shell-specific
+  prefix.
+- The matched substring itself, including the number at the end,
+  starting with 'RUN', skipping the shell-specific prefix.

yln wrote:
> Why use a complicated parser-like return value? Our only caller below could 
> just receive the potentially found RUN line.
Sorry, it's not super clear because in this revision, there is only one caller, 
which could do with just using the index. However, the highlighting patch 
(which comes right after this logically) ends up making use of the substring. 
The line of reasoning is:

1. A substring by itself is not good enough (and hence `make_command_output` in 
this patch makes use of the first index return value), because we want to 
distinguish the case `line_start == 0` from the case `line_start > 0` by 
printing `Command Output (stderr)` instead of `Command Output (stderr, 
truncated)` when `line_start == 0`.
2. An index by itself could be "good enough", but we try to be smarter in the 
highlighting patch (https://reviews.llvm.org/D82811) by not highlighting the 
shell-specific prefix, and starting highlighting from the "RUN", which 
`make_script_output` makes use of.

Does that make sense? I decided against splitting changes to this function into 
two revisions and have the full functionality in the first iteration because I 
felt like that would create more churn with the tests and the implementation, 
but I see how it can be confusing as to why two 
related-but-slightly-different-values are being returned simultaneously when 
only one is really being used in this patch.



Comment at: llvm/utils/lit/lit/TestRunner.py:1593
+assert(lit_config.script_output_style == OutputSettings.FULL_SCRIPT)
+return default_output()
+

yln wrote:
> Why are we using two local functions here?
> 
> The whole thing could be (already assuming just one verbose flag):
> 
> ```
> def make_script_output(lit_config, script_lines, exit_code):
> if not lit_config.verbose:
> return ""
> return ...
> ```
It mimics the structure of the `make_command_output` function right below. 
Right now, this function is very simple, so it probably seems overkill. 
However, `make_script_output` becomes much more complicated in the highlighting 
patch after this: https://reviews.llvm.org/D82811, so I figured it would be 
helpful to have the two functions `make_script_output` and 
`make_command_output` follow a similar pa

[PATCH] D82791: [lit] Improve lit's output with default settings and --verbose.

[Varun Gandhi via Phabricator via cfe-commits]

varungandhi-apple updated this revision to Diff 280786.
varungandhi-apple marked 11 inline comments as done.
varungandhi-apple added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82791

Files:
  llvm/docs/CommandGuide/lit.rst
  llvm/utils/lit/lit/LitConfig.py
  llvm/utils/lit/lit/OutputSettings.py
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/lit/cl_arguments.py
  llvm/utils/lit/lit/display.py
  llvm/utils/lit/lit/main.py
  llvm/utils/lit/tests/shtest-format.py
  llvm/utils/lit/tests/shtest-run-at-line.py
  llvm/utils/lit/tests/unittest-failing-locator.py

Index: llvm/utils/lit/tests/unittest-failing-locator.py
===
--- /dev/null
+++ llvm/utils/lit/tests/unittest-failing-locator.py
@@ -0,0 +1,122 @@
+# Check that the locate_last_failing_run_line function works as expected.
+
+# RUN: %{python} %s
+# END.
+
+import unittest
+
+from lit.TestRunner import locate_last_run_line
+
+class TestRunLineLocatorHeuristics(unittest.TestCase):
+
+def test_basic(self):
+# from a script like
+# RUN: echo Hello 1>&2
+basic = (
+"+ : 'RUN: at line 1'\n"
+"+ echo Hello 1>&2\n"
+"+ Hello\n"
+)
+line_start, substr = locate_last_run_line(basic)
+self.assertEqual(line_start, 0)
+self.assertEqual(substr, "RUN: at line 1")
+
+def test_multiple(self):
+# from a script like
+# RUN: echo Hello 1>&2
+# RUN: false
+multiple = (
+"+ : 'RUN: at line 1'\n"
+"+ echo Hello 1>&2\n"
+"+ Hello\n"
+"+ : 'RUN: at line 2'\n"
+"+ false\n"
+)
+line_start, substr = locate_last_run_line(multiple)
+self.assertEqual(line_start, multiple.rfind("+ :"))
+self.assertEqual(substr, "RUN: at line 2")
+
+def test_varying_prefix(self):
+# from a script like
+# RUN: echo Hello 1>&2
+# RUN: false
+#
+# in a hypothetical shell which prints line-numbers on 'set +x'
+# (as an example of something that varies)
+varying_prefix = (
+"+ 1 : 'RUN: at line 1'\n"
+"+ 2 echo Hello 1>&2\n"
+"+ Hello\n"
+"+ 3 : 'RUN: at line 2'\n"
+"+ 4 false\n"
+)
+line_start, substr = locate_last_run_line(varying_prefix)
+self.assertEqual(line_start, varying_prefix.rfind("+ 3"))
+self.assertEqual(substr, "RUN: at line 2")
+
+def test_confusing_basic(self):
+# from a script like
+# RUN: echo 'RUN: at line 10' 1>&2
+confusing_basic = (
+"+ : 'RUN: at line 1'\n"
+"+ echo 'RUN: at line 10'\n"
+"RUN: at line 10\n"
+)
+line_start, substr = locate_last_run_line(confusing_basic)
+# FIXME: These should both be equal ideally.
+self.assertNotEqual(line_start, 0)
+self.assertNotEqual(substr, "RUN: at line 1")
+
+def test_confusing_multiple(self):
+# from a script like
+# RUN: echo 'RUN: at line 10' 1>&2
+# RUN: false
+confusing_multiple = (
+"+ : 'RUN: at line 1'\n"
+"+ echo 'RUN: at line 10'\n"
+"RUN: at line 10\n"
+"+ : 'RUN: at line 2'\n"
+"+ false\n"
+)
+line_start, substr = locate_last_run_line(confusing_multiple)
+self.assertEqual(line_start, confusing_multiple.rfind("+ :"))
+self.assertEqual(substr, "RUN: at line 2")
+
+def test_confusing_varying_prefix_1(self):
+# from a script like
+# RUN: echo 'RUN: at line 10' 1>&2
+# RUN: false
+#
+# in a hypothetical shell which prints line-numbers on 'set +x'
+# (as an example of something that varies)
+confusing_varying_prefix = (
+"+ 1 : 'RUN: at line 1'\n"
+"+ 2 echo 'RUN: at line 10'\n"
+"RUN: at line 10\n"
+"+ 3 : 'RUN: at line 2'\n"
+"+ 4 false\n"
+)
+line_start, substr = locate_last_run_line(confusing_varying_prefix)
+self.assertEqual(line_start, confusing_varying_prefix.rfind("+ 3"))
+self.assertEqual(substr, "RUN: at line 2")
+
+def test_confusing_varying_prefix_2(self):
+# from a script like
+# RUN: true
+# RUN: not echo 'RUN: at line 100'
+#
+# in a hypothetical shell which prints line-numbers on 'set +x'
+# (as an example of something that varies)
+confusing_varying_prefix = (
+"+ 1 : 'RUN: at line 1'\n"
+"+ 2 true\n"
+"+ 3 : 'RUN: at line 2'\n"
+"+ 4 not echo 'RUN: at line 100'\n"
+"+ RUN: at line 100\n"
+)
+line_start, substr = locate_last_run_line(confusing_varying_prefix)

[PATCH] D84136: [clangd] Fix visitation of ConceptSpecializationExpr in constrained-parameter

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 280771.
nridge marked an inline comment as done.
nridge added a comment.

Spun off the source-location issue to D84613 .

Some other review comments remain to be addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84136

Files:
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang/include/clang/AST/RecursiveASTVisitor.h


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -1777,8 +1777,10 @@
   // D is the "T" in something like "template class vector;"
   if (D->getTypeForDecl())
 TRY_TO(TraverseType(QualType(D->getTypeForDecl(), 0)));
-  if (const auto *TC = D->getTypeConstraint())
+  if (const auto *TC = D->getTypeConstraint()) {
+TRY_TO(TraverseStmt(TC->getImmediatelyDeclaredConstraint()));
 TRY_TO(TraverseConceptReference(*TC));
+  }
   if (D->hasDefaultArgument() && !D->defaultArgumentWasInherited())
 TRY_TO(TraverseTypeLoc(D->getDefaultArgumentInfo()->getTypeLoc()));
 })
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -405,6 +405,11 @@
 }
 
 TEST_F(TargetDeclTest, Concept) {
+  Flags.push_back("-std=c++20");
+
+  // FIXME: Should we truncate the pretty-printed form of a concept decl
+  // somewhere?
+
   Code = R"cpp(
 template 
 concept Fooable = requires (T t) { t.foo(); };
@@ -414,12 +419,31 @@
   t.foo();
 }
   )cpp";
-  Flags.push_back("-std=c++20");
   EXPECT_DECLS(
   "ConceptSpecializationExpr",
-  // FIXME: Should we truncate the pretty-printed form of a concept decl
-  // somewhere?
   {"template  concept Fooable = requires (T t) { t.foo(); 
};"});
+
+  // constrained-parameter
+  Code = R"cpp(
+template 
+concept Fooable = true;
+
+template <[[Fooable]] T>
+void bar(T t);
+  )cpp";
+  EXPECT_DECLS("ConceptSpecializationExpr",
+   {"template  concept Fooable = true;"});
+
+  // partial-concept-id
+  Code = R"cpp(
+template 
+concept Fooable = true;
+
+template <[[Fooable]] T>
+void bar(T t);
+  )cpp";
+  EXPECT_DECLS("ConceptSpecializationExpr",
+   {"template  concept Fooable = true;"});
 }
 
 TEST_F(TargetDeclTest, FunctionTemplate) {


Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -1777,8 +1777,10 @@
   // D is the "T" in something like "template class vector;"
   if (D->getTypeForDecl())
 TRY_TO(TraverseType(QualType(D->getTypeForDecl(), 0)));
-  if (const auto *TC = D->getTypeConstraint())
+  if (const auto *TC = D->getTypeConstraint()) {
+TRY_TO(TraverseStmt(TC->getImmediatelyDeclaredConstraint()));
 TRY_TO(TraverseConceptReference(*TC));
+  }
   if (D->hasDefaultArgument() && !D->defaultArgumentWasInherited())
 TRY_TO(TraverseTypeLoc(D->getDefaultArgumentInfo()->getTypeLoc()));
 })
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -405,6 +405,11 @@
 }
 
 TEST_F(TargetDeclTest, Concept) {
+  Flags.push_back("-std=c++20");
+
+  // FIXME: Should we truncate the pretty-printed form of a concept decl
+  // somewhere?
+
   Code = R"cpp(
 template 
 concept Fooable = requires (T t) { t.foo(); };
@@ -414,12 +419,31 @@
   t.foo();
 }
   )cpp";
-  Flags.push_back("-std=c++20");
   EXPECT_DECLS(
   "ConceptSpecializationExpr",
-  // FIXME: Should we truncate the pretty-printed form of a concept decl
-  // somewhere?
   {"template  concept Fooable = requires (T t) { t.foo(); };"});
+
+  // constrained-parameter
+  Code = R"cpp(
+template 
+concept Fooable = true;
+
+template <[[Fooable]] T>
+void bar(T t);
+  )cpp";
+  EXPECT_DECLS("ConceptSpecializationExpr",
+   {"template  concept Fooable = true;"});
+
+  // partial-concept-id
+  Code = R"cpp(
+template 
+concept Fooable = true;
+
+template <[[Fooable]] T>
+void bar(T t);
+  )cpp";
+  EXPECT_DECLS("ConceptSpecializationExpr",
+   {"template  concept Fooable = true;"});
 }
 
 TEST_F(TargetDeclTest, FunctionTemplate) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84613: [clang] Fix ConceptSpecializationExpr::getEndLoc()

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: hokein.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

It returned an invalid location in case of a constrained-parameter
with no explicit arguments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84613

Files:
  clang/include/clang/AST/ExprConcepts.h
  clang/test/AST/ast-dump-concepts.cpp


Index: clang/test/AST/ast-dump-concepts.cpp
===
--- clang/test/AST/ast-dump-concepts.cpp
+++ clang/test/AST/ast-dump-concepts.cpp
@@ -1,5 +1,8 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++2a -ast-dump 
-ast-dump-filter Foo %s | FileCheck -strict-whitespace %s
 
+template 
+concept unary_concept = true;
+
 template 
 concept not_same_as = true;
 
@@ -10,4 +13,9 @@
   // CHECK-NEXT: `-TemplateArgument {{.*}} type 'int'
   template  R>
   Foo(R) requires(true);
+
+  // CHECK:  TemplateTypeParmDecl {{.*}} referenced Concept {{.*}} 
'unary_concept'
+  // CHECK-NEXT: `-ConceptSpecializationExpr {{.*}}  'bool'
+  template 
+  Foo(R);
 };
Index: clang/include/clang/AST/ExprConcepts.h
===
--- clang/include/clang/AST/ExprConcepts.h
+++ clang/include/clang/AST/ExprConcepts.h
@@ -126,7 +126,11 @@
   }
 
   SourceLocation getEndLoc() const LLVM_READONLY {
-return ArgsAsWritten->RAngleLoc;
+// If the ConceptSpecializationExpr is the ImmediatelyDeclaredConstraint
+// of a TypeConstraint written syntactically as a constrained-parameter,
+// there may not be a template argument list.
+return ArgsAsWritten->RAngleLoc.isValid() ? ArgsAsWritten->RAngleLoc
+  : ConceptName.getEndLoc();
   }
 
   // Iterators


Index: clang/test/AST/ast-dump-concepts.cpp
===
--- clang/test/AST/ast-dump-concepts.cpp
+++ clang/test/AST/ast-dump-concepts.cpp
@@ -1,5 +1,8 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++2a -ast-dump -ast-dump-filter Foo %s | FileCheck -strict-whitespace %s
 
+template 
+concept unary_concept = true;
+
 template 
 concept not_same_as = true;
 
@@ -10,4 +13,9 @@
   // CHECK-NEXT: `-TemplateArgument {{.*}} type 'int'
   template  R>
   Foo(R) requires(true);
+
+  // CHECK:  TemplateTypeParmDecl {{.*}} referenced Concept {{.*}} 'unary_concept'
+  // CHECK-NEXT: `-ConceptSpecializationExpr {{.*}}  'bool'
+  template 
+  Foo(R);
 };
Index: clang/include/clang/AST/ExprConcepts.h
===
--- clang/include/clang/AST/ExprConcepts.h
+++ clang/include/clang/AST/ExprConcepts.h
@@ -126,7 +126,11 @@
   }
 
   SourceLocation getEndLoc() const LLVM_READONLY {
-return ArgsAsWritten->RAngleLoc;
+// If the ConceptSpecializationExpr is the ImmediatelyDeclaredConstraint
+// of a TypeConstraint written syntactically as a constrained-parameter,
+// there may not be a template argument list.
+return ArgsAsWritten->RAngleLoc.isValid() ? ArgsAsWritten->RAngleLoc
+  : ConceptName.getEndLoc();
   }
 
   // Iterators
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84613: [clang] Fix ConceptSpecializationExpr::getEndLoc()

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Spun off from D84136 . Depends on D84461 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84613



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

[clang] 0eff8b3 - [PowerPC] Cleanup p10vector clang test

[via cfe-commits]

Author: biplmish
Date: 2020-07-26T21:23:00-05:00
New Revision: 0eff8b3865ede487bacd605f628891dd028c74bd

URL: 
https://github.com/llvm/llvm-project/commit/0eff8b3865ede487bacd605f628891dd028c74bd
DIFF: 
https://github.com/llvm/llvm-project/commit/0eff8b3865ede487bacd605f628891dd028c74bd.diff

LOG: [PowerPC] Cleanup p10vector clang test

Remove the duplicate LE test, correct the labels and remove common tests for 
vec_splat builtin.

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

Added: 


Modified: 
clang/test/CodeGen/builtins-ppc-p10vector.c

Removed: 




diff  --git a/clang/test/CodeGen/builtins-ppc-p10vector.c 
b/clang/test/CodeGen/builtins-ppc-p10vector.c
index 2182a19f2452..e67018b06214 100644
--- a/clang/test/CodeGen/builtins-ppc-p10vector.c
+++ b/clang/test/CodeGen/builtins-ppc-p10vector.c
@@ -1,15 +1,11 @@
 // REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -target-feature +vsx -target-feature +altivec \
-// RUN:   -target-cpu pwr10 -triple powerpc64le-unknown-unknown -emit-llvm %s \
-// RUN:   -o - | FileCheck %s
-
-// RUN: %clang_cc1 -target-feature +vsx -target-feature +altivec \
+// RUN: %clang_cc1 -target-feature +vsx \
 // RUN:   -target-cpu pwr10 -triple powerpc64-unknown-unknown -emit-llvm %s \
-// RUN:   -o - | FileCheck %s -check-prefix=CHECK-BE
+// RUN:   -o - | FileCheck %s -check-prefixes=CHECK-BE,CHECK
 
-// RUN: %clang_cc1 -target-feature +vsx -target-feature +altivec \
+// RUN: %clang_cc1 -target-feature +vsx \
 // RUN:   -target-cpu pwr10 -triple powerpc64le-unknown-unknown -emit-llvm %s \
-// RUN:   -o - | FileCheck %s -check-prefix=CHECK-LE
+// RUN:   -o - | FileCheck %s -check-prefixes=CHECK-LE,CHECK
 
 #include 
 
@@ -514,19 +510,16 @@ vector unsigned int test_vec_inserth_uiv(void) {
 }
 
 vector signed int test_vec_vec_splati_si(void) {
-  // CHECK-BE: ret <4 x i32> 
   // CHECK: ret <4 x i32> 
   return vec_splati(-17);
 }
 
 vector unsigned int test_vec_vec_splati_ui(void) {
-  // CHECK-BE: ret <4 x i32> 
   // CHECK: ret <4 x i32> 
   return vec_splati(16U);
 }
 
 vector float test_vec_vec_splati_f(void) {
-  // CHECK-BE: ret <4 x float> 
   // CHECK: ret <4 x float> 
   return vec_splati(1.0f);
 }
@@ -536,10 +529,10 @@ vector double test_vec_vec_splatid(void) {
   // CHECK-BE-NEXT: [[T2:%.+]] = insertelement <2 x double> undef, double 
[[T1:%.+]], i32 0
   // CHECK-BE-NEXT: [[T3:%.+]] = shufflevector <2 x double> [[T2:%.+]], <2 x 
double> undef, <2 x i32> zeroinitialize
   // CHECK-BE-NEXT: ret <2 x double> [[T3:%.+]]
-  // CHECK: [[T1:%.+]] = fpext float %{{.+}} to double
-  // CHECK-NEXT: [[T2:%.+]] = insertelement <2 x double> undef, double 
[[T1:%.+]], i32 0
-  // CHECK-NEXT: [[T3:%.+]] = shufflevector <2 x double> [[T2:%.+]], <2 x 
double> undef, <2 x i32> zeroinitialize
-  // CHECK-NEXT: ret <2 x double> [[T3:%.+]]
+  // CHECK-LE: [[T1:%.+]] = fpext float %{{.+}} to double
+  // CHECK-LE-NEXT: [[T2:%.+]] = insertelement <2 x double> undef, double 
[[T1:%.+]], i32 0
+  // CHECK-LE-NEXT: [[T3:%.+]] = shufflevector <2 x double> [[T2:%.+]], <2 x 
double> undef, <2 x i32> zeroinitialize
+  // CHECK-LE-NEXT: ret <2 x double> [[T3:%.+]]
   return vec_splatid(1.0);
 }
 
@@ -548,11 +541,11 @@ vector signed int test_vec_vec_splati_ins_si(void) {
   // CHECK-BE:  [[T1:%.+]] = add i32 2, %{{.+}}
   // CHECK-BE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
   // CHECK-BE: ret <4 x i32>
-  // CHECK:  [[T1:%.+]] = sub i32 1, %{{.+}}
-  // CHECK: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
-  // CHECK:  [[T2:%.+]] = sub i32 3, %{{.+}}
-  // CHECK: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T2]]
-  // CHECK: ret <4 x i32>
+  // CHECK-LE:  [[T1:%.+]] = sub i32 1, %{{.+}}
+  // CHECK-LE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
+  // CHECK-LE:  [[T2:%.+]] = sub i32 3, %{{.+}}
+  // CHECK-LE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T2]]
+  // CHECK-LE: ret <4 x i32>
   return vec_splati_ins(vsia, 0, -17);
 }
 
@@ -561,11 +554,11 @@ vector unsigned int test_vec_vec_splati_ins_ui(void) {
   // CHECK-BE:  [[T1:%.+]] = add i32 2, %{{.+}}
   // CHECK-BE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
   // CHECK-BE: ret <4 x i32>
-  // CHECK:  [[T1:%.+]] = sub i32 1, %{{.+}}
-  // CHECK: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
-  // CHECK:  [[T2:%.+]] = sub i32 3, %{{.+}}
-  // CHECK: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T2]]
-  // CHECK: ret <4 x i32>
+  // CHECK-LE:  [[T1:%.+]] = sub i32 1, %{{.+}}
+  // CHECK-LE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
+  // CHECK-LE:  [[T2:%.+]] = sub i32 3, %{{.+}}
+  // CHECK-LE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T2]]
+  // CHECK-LE: ret <4 x i32>
   return vec_splati_ins(vuia, 1, 16U);
 }
 
@@ -574,11 +567,11 @@ vector float test_vec_vec_splati_ins_f(void) {
   // CHECK-BE:  [[T1:%.+]] = add i32 2, %{{.+}}

[PATCH] D84382: [PowerPC][Power10] Cleanup p10vector clang test

[Biplob Mishra via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0eff8b3865ed: [PowerPC] Cleanup p10vector clang test 
(authored by biplmish).

Changed prior to commit:
  https://reviews.llvm.org/D84382?vs=280022&id=280769#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84382

Files:
  clang/test/CodeGen/builtins-ppc-p10vector.c

Index: clang/test/CodeGen/builtins-ppc-p10vector.c
===
--- clang/test/CodeGen/builtins-ppc-p10vector.c
+++ clang/test/CodeGen/builtins-ppc-p10vector.c
@@ -1,15 +1,11 @@
 // REQUIRES: powerpc-registered-target
-// RUN: %clang_cc1 -target-feature +vsx -target-feature +altivec \
-// RUN:   -target-cpu pwr10 -triple powerpc64le-unknown-unknown -emit-llvm %s \
-// RUN:   -o - | FileCheck %s
-
-// RUN: %clang_cc1 -target-feature +vsx -target-feature +altivec \
+// RUN: %clang_cc1 -target-feature +vsx \
 // RUN:   -target-cpu pwr10 -triple powerpc64-unknown-unknown -emit-llvm %s \
-// RUN:   -o - | FileCheck %s -check-prefix=CHECK-BE
+// RUN:   -o - | FileCheck %s -check-prefixes=CHECK-BE,CHECK
 
-// RUN: %clang_cc1 -target-feature +vsx -target-feature +altivec \
+// RUN: %clang_cc1 -target-feature +vsx \
 // RUN:   -target-cpu pwr10 -triple powerpc64le-unknown-unknown -emit-llvm %s \
-// RUN:   -o - | FileCheck %s -check-prefix=CHECK-LE
+// RUN:   -o - | FileCheck %s -check-prefixes=CHECK-LE,CHECK
 
 #include 
 
@@ -514,19 +510,16 @@
 }
 
 vector signed int test_vec_vec_splati_si(void) {
-  // CHECK-BE: ret <4 x i32> 
   // CHECK: ret <4 x i32> 
   return vec_splati(-17);
 }
 
 vector unsigned int test_vec_vec_splati_ui(void) {
-  // CHECK-BE: ret <4 x i32> 
   // CHECK: ret <4 x i32> 
   return vec_splati(16U);
 }
 
 vector float test_vec_vec_splati_f(void) {
-  // CHECK-BE: ret <4 x float> 
   // CHECK: ret <4 x float> 
   return vec_splati(1.0f);
 }
@@ -536,10 +529,10 @@
   // CHECK-BE-NEXT: [[T2:%.+]] = insertelement <2 x double> undef, double [[T1:%.+]], i32 0
   // CHECK-BE-NEXT: [[T3:%.+]] = shufflevector <2 x double> [[T2:%.+]], <2 x double> undef, <2 x i32> zeroinitialize
   // CHECK-BE-NEXT: ret <2 x double> [[T3:%.+]]
-  // CHECK: [[T1:%.+]] = fpext float %{{.+}} to double
-  // CHECK-NEXT: [[T2:%.+]] = insertelement <2 x double> undef, double [[T1:%.+]], i32 0
-  // CHECK-NEXT: [[T3:%.+]] = shufflevector <2 x double> [[T2:%.+]], <2 x double> undef, <2 x i32> zeroinitialize
-  // CHECK-NEXT: ret <2 x double> [[T3:%.+]]
+  // CHECK-LE: [[T1:%.+]] = fpext float %{{.+}} to double
+  // CHECK-LE-NEXT: [[T2:%.+]] = insertelement <2 x double> undef, double [[T1:%.+]], i32 0
+  // CHECK-LE-NEXT: [[T3:%.+]] = shufflevector <2 x double> [[T2:%.+]], <2 x double> undef, <2 x i32> zeroinitialize
+  // CHECK-LE-NEXT: ret <2 x double> [[T3:%.+]]
   return vec_splatid(1.0);
 }
 
@@ -548,11 +541,11 @@
   // CHECK-BE:  [[T1:%.+]] = add i32 2, %{{.+}}
   // CHECK-BE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
   // CHECK-BE: ret <4 x i32>
-  // CHECK:  [[T1:%.+]] = sub i32 1, %{{.+}}
-  // CHECK: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
-  // CHECK:  [[T2:%.+]] = sub i32 3, %{{.+}}
-  // CHECK: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T2]]
-  // CHECK: ret <4 x i32>
+  // CHECK-LE:  [[T1:%.+]] = sub i32 1, %{{.+}}
+  // CHECK-LE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
+  // CHECK-LE:  [[T2:%.+]] = sub i32 3, %{{.+}}
+  // CHECK-LE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T2]]
+  // CHECK-LE: ret <4 x i32>
   return vec_splati_ins(vsia, 0, -17);
 }
 
@@ -561,11 +554,11 @@
   // CHECK-BE:  [[T1:%.+]] = add i32 2, %{{.+}}
   // CHECK-BE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
   // CHECK-BE: ret <4 x i32>
-  // CHECK:  [[T1:%.+]] = sub i32 1, %{{.+}}
-  // CHECK: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
-  // CHECK:  [[T2:%.+]] = sub i32 3, %{{.+}}
-  // CHECK: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T2]]
-  // CHECK: ret <4 x i32>
+  // CHECK-LE:  [[T1:%.+]] = sub i32 1, %{{.+}}
+  // CHECK-LE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T1]]
+  // CHECK-LE:  [[T2:%.+]] = sub i32 3, %{{.+}}
+  // CHECK-LE: insertelement <4 x i32> %{{.+}}, i32 %{{.+}}, i32 [[T2]]
+  // CHECK-LE: ret <4 x i32>
   return vec_splati_ins(vuia, 1, 16U);
 }
 
@@ -574,11 +567,11 @@
   // CHECK-BE:  [[T1:%.+]] = add i32 2, %{{.+}}
   // CHECK-BE: insertelement <4 x float> %{{.+}}, float %{{.+}}, i32 [[T1]]
   // CHECK-BE: ret <4 x float>
-  // CHECK:  [[T1:%.+]] = sub i32 1, %{{.+}}
-  // CHECK: insertelement <4 x float> %{{.+}}, float %{{.+}}, i32 [[T1]]
-  // CHECK:  [[T2:%.+]] = sub i32 3, %{{.+}}
-  // CHECK: insertelement <4 x float> %{{.+}}, float %{{.+}}, i32 [[T2]]
-  // CHECK: ret <4 x float>
+  // CHECK-LE:  [[T1:%.+]] = sub i32 1, %{{.+}}
+  // CHECK-LE: insertelement <4 x float> %{{.+}}, float %{{.+}}, i32 [[T

[PATCH] D84461: [Concepts] Fix ast dump for immediately declared constraint.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang/test/AST/ast-dump-concepts.cpp:12
+  template  R>
+  Foo(R) requires(true);
+};

another nit: the `requires(true)` is not relevant to the testcase.

(Although, perhaps it is another bug that it does not appear in the dump?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84461



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

[PATCH] D84461: [Concepts] Fix ast dump for immediately declared constraint.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang/test/AST/ast-dump-concepts.cpp:4
+template 
+concept not_same_as = true;
+

nit: perhaps use a more generic name like `binary_concept` (meaning concept 
that takes two types)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84461



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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179
+
+  switch (SmartPtrMethod) {
+  case Constructor: {

NoQ wrote:
> I feel this is a bit over-engineered. There's no need to create an enum and 
> later convert it into a string when you can capture a string directly. Like, 
> this entire "details" structure of your, it should be just captures instead, 
> and your strings would make it immediately obvious what kind of note is 
> emitted:
> ```lang=c++
> C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
>   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> return "";
>  
>   return Twine("Default constructed smart pointer '") + getSmartPtrName(R) + 
> "' is null";
> }));
> ```
> 
> Hmm, maybe we should also provide an overload with lambdas of 
> signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that to 
> make the same one-liners possible with streams? Something like this:
> 
> ```lang=c++
> class CheckerContext {
> // ...
>   NoteTag *getNoteTag(std::function llvm::raw_ostream OS)> f) {
> return getNoteTag([](PathSensitiveBugReport &BR) -> std::string {
>   llvm::SmallString<128> Str;
>   llvm::raw_svector_ostream OS(Str);
>   f(BR, OS);
>   return OS.str();
> });
>   }
> // ...
> };
> ```
> 
> Then you could do:
> ```lang=c++
> C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
>   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> return;
> 
>   OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' is 
> null";
> }));
> ```
(forgot `, llvm::raw_ostream OS` in the last snippet; probably forgot a bunch 
of other stuff because i didn't try to actually compile these snippets)



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

NoQ wrote:
> Ok, note that note tags are attached to nodes independently of bug reports; 
> when the report is thrown, only then we know what are the smart pointers that 
> should be explained.
> 
> So there are two checks that you should do here:
> 
> 1. Check that the bug report is emitted by your checker (eg., by comparing 
> bug types). If not, don't add notes.
> 
> 2. Check that the region about which the note speaks is related to your 
> report (i.e., it's not a completely unrelated smart pointer). You can do that 
> by marking the smart pointer as "interesting" (i.e., 
> `PathSensitiveBugReport::markIntersting()`) when you emit the report, and 
> then in the lambda you check whether the smart pointer is interesting before 
> you emit a note. Additionally, you can carry over interestingness when smart 
> pointers are copied.
> 
> This is what i was trying to accomplish with this code snippet that i 
> included in the examples in the other comment:
> ```lang=c++
>   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> return "";
> ```
(i strongly recommend having test cases for both of these issues)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600



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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:86
+  if (const auto *DR = dyn_cast(DerefRegion)) {
+auto SmartPtrName = DR->getDecl()->getName();
+OS << " '" << SmartPtrName << "'";

Please `getNameAsString()` because `getName()` crashes on anonymous 
declarations (eg., lambda captures, anonymous nested structs/unions, etc.).



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:95-96
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (RecordDecl && RecordDecl->getDeclName().isIdentifier()) {
+OS << " of type '" << RecordDecl->getQualifiedNameAsString() << "'";

Aha, this time you checked for anonymous declarations! Good.

That said, i'm not sure type is actually useful for this warning, because 
they're roughly all the same.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179
+
+  switch (SmartPtrMethod) {
+  case Constructor: {

I feel this is a bit over-engineered. There's no need to create an enum and 
later convert it into a string when you can capture a string directly. Like, 
this entire "details" structure of your, it should be just captures instead, 
and your strings would make it immediately obvious what kind of note is emitted:
```lang=c++
C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return "";
 
  return Twine("Default constructed smart pointer '") + getSmartPtrName(R) + "' 
is null";
}));
```

Hmm, maybe we should also provide an overload with lambdas of 
signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that to 
make the same one-liners possible with streams? Something like this:

```lang=c++
class CheckerContext {
// ...
  NoteTag *getNoteTag(std::function f) {
return getNoteTag([](PathSensitiveBugReport &BR) -> std::string {
  llvm::SmallString<128> Str;
  llvm::raw_svector_ostream OS(Str);
  f(BR, OS);
  return OS.str();
});
  }
// ...
};
```

Then you could do:
```lang=c++
C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return;

  OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' is 
null";
}));
```



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+SmallString<128> Msg;
+llvm::raw_svector_ostream Out(Msg);
+TagDetails.trackValidExpr(BR);
+TagDetails.explainSmartPtrAction(Out);
+return std::string(Out.str());

Ok, note that note tags are attached to nodes independently of bug reports; 
when the report is thrown, only then we know what are the smart pointers that 
should be explained.

So there are two checks that you should do here:

1. Check that the bug report is emitted by your checker (eg., by comparing bug 
types). If not, don't add notes.

2. Check that the region about which the note speaks is related to your report 
(i.e., it's not a completely unrelated smart pointer). You can do that by 
marking the smart pointer as "interesting" (i.e., 
`PathSensitiveBugReport::markIntersting()`) when you emit the report, and then 
in the lambda you check whether the smart pointer is interesting before you 
emit a note. Additionally, you can carry over interestingness when smart 
pointers are copied.

This is what i was trying to accomplish with this code snippet that i included 
in the examples in the other comment:
```lang=c++
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
return "";
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600



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

[PATCH] D84136: [clangd] Fix visitation of ConceptSpecializationExpr in constrained-parameter

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.



Comment at: clang/include/clang/AST/ExprConcepts.h:132
+// there may not be a template argument list.
+return ArgsAsWritten->RAngleLoc.isValid() ? ArgsAsWritten->RAngleLoc
+  : ConceptName.getEndLoc();

hokein wrote:
> kadircet wrote:
> > i think we should have some tests in clang, at least an ast-dump test in 
> > `clang/test/AST/` (for both cases) and possibly also in 
> > `clang/unittests/AST/SourceLocationTest.cpp`
> +1, ast-dump should be enough to test the invalid end loc, I have a D84461 to 
> slightly improve the dump.
Will do. By the way, is there something more tailored than `ninja check-clang` 
to run these ast-dump tests? `ninja check-clang` takes quite a while to run...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84136



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

[PATCH] D83592: [Coverage] Add comment to skipped regions

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

In D83592#2174600 , @vsk wrote:

> In D83592#2170912 , @zequanwu wrote:
>
> > I don't why when using the cmake option 
> > `-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On` to test coverage for clang itself, 
> > it does tracking comments.
> >  Also, with that option on, `llvm-cov` crashes at 
> > assertion:`llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:578: 
> > virtual Expected (anonymous 
> > namespace)::VersionedCovMapFuncRecordReader > unsigned long, llvm::support::little>::readCoverageHeader(const char *, 
> > const char *, BinaryCoverageReader::DecompressedData &) [Version = 
> > llvm::coverage::Version4, IntPtrT = unsigned long, Endian = 
> > llvm::support::little]: Assertion 
> > `(CovMapVersion)CovHeader->getVersion() == Version' failed.`
>
>
> Just to double-check: are you using llvm-{cov,profdata} binaries from the 
> stage1 llvm build to produce reports for stage2 (coverage-instrumented) 
> binaries?


I didn't config correctly before. I just tried. It works correctly now, not 
tracking comments.


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

https://reviews.llvm.org/D83592



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

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

A macro for wavefront size would make targeting gfx10 for openmp easier.

We currently use an int32_t for nvptx and an int64_t for amdgcn in various 
runtime function interfaces. I'd like to be able to set the latter based on 
said macro.


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

https://reviews.llvm.org/D82087



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

[PATCH] D83592: [Coverage] Add comment to skipped regions

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D83592#2170912 , @zequanwu wrote:

> I don't why when using the cmake option 
> `-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On` to test coverage for clang itself, it 
> does tracking comments.
>  Also, with that option on, `llvm-cov` crashes at 
> assertion:`llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:578: 
> virtual Expected (anonymous 
> namespace)::VersionedCovMapFuncRecordReader unsigned long, llvm::support::little>::readCoverageHeader(const char *, const 
> char *, BinaryCoverageReader::DecompressedData &) [Version = 
> llvm::coverage::Version4, IntPtrT = unsigned long, Endian = 
> llvm::support::little]: Assertion 
> `(CovMapVersion)CovHeader->getVersion() == Version' failed.`


Just to double-check: are you using llvm-{cov,profdata} binaries from the 
stage1 llvm build to produce reports for stage2 (coverage-instrumented) 
binaries?


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

https://reviews.llvm.org/D83592



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

[PATCH] D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a reviewer: zequanwu.
vsk added subscribers: ikudrin, arphaman.
vsk added a comment.

@alanphipps thanks for the patch! I'm a bit swamped at the moment, but I hope 
to give a detailed review by this Wednesday.




Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+  /// condition (i.e. no "&&" or "||").
+  static bool isLeafCondition(const Expr *C);
+

It might be helpful to either require that `C` be the RHS of a logical binop 
(e.g. by passing the binop expr in), or to document that requirement.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+  /// condition (i.e. no "&&" or "||").
+  static bool isLeafCondition(const Expr *C);
+

vsk wrote:
> It might be helpful to either require that `C` be the RHS of a logical binop 
> (e.g. by passing the binop expr in), or to document that requirement.
Given a logical binop like `E = a && !(b || c)`, `isLeafCondition(E->getRHS())` 
is true. That seems a bit counter-intuitive, because `E->getRHS()` contains 
another leaf condition. Would it make sense to rename the condition (perhaps to 
something like 'InstrumentedCondition')? Have I misunderstood what a leaf 
condition is?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84467



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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Test failure is expected because I based this on D84603 
 locally, but I'm not sure how to tell that to 
Phabricator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604



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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a subscriber: anarazel.
aaronpuchert added a comment.

@anarazel, that should fix the issue you reported on IRC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604



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

[PATCH] D84499: [clangd] Add more logs and attach tracers to remote index server routines

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Need to resolve remaining comments and improve readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84499



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

[PATCH] D84535: [clangd] Switch from EXPECT_TRUE to ASSERT_TRUE in remote marshalling tests

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280745.
kbobyrev added a comment.

Find one more case when checking directly makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84535

Files:
  clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
@@ -49,11 +49,11 @@
   "clangd/unittests/remote/MarshallingTests.cpp",
   Strings);
   auto Serialized = ProtobufMarshaller.toProtobuf(Original);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   EXPECT_EQ(Serialized->location().file_path(),
 "clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp");
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
   EXPECT_STREQ(Deserialized->Location.FileURI,
testPathURI("home/my-projects/llvm-project/clang-tools-extra/"
"clangd/unittests/remote/MarshallingTests.cpp",
@@ -61,38 +61,34 @@
 
   // Can't have empty paths.
   *Serialized->mutable_location()->mutable_file_path() = std::string();
-  Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized));
 
   clangd::Ref WithInvalidURI;
   // Invalid URI results in serialization failure.
   WithInvalidURI.Location.FileURI = "This is not a URI";
-  Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(WithInvalidURI));
 
   // Can not use URIs with scheme different from "file".
   auto UnittestURI =
   URI::create(testPath("project/lib/HelloWorld.cpp"), "unittest");
-  EXPECT_TRUE(bool(UnittestURI));
+  ASSERT_TRUE(bool(UnittestURI));
   WithInvalidURI.Location.FileURI =
   Strings.save(UnittestURI->toString()).begin();
-  Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(WithInvalidURI));
 
   // Paths transmitted over the wire can not be absolute, they have to be
   // relative.
   Ref WithAbsolutePath;
   *WithAbsolutePath.mutable_location()->mutable_file_path() =
   "/usr/local/user/home/HelloWorld.cpp";
-  Deserialized = ProtobufMarshaller.fromProtobuf(WithAbsolutePath);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(WithAbsolutePath));
 }
 
 TEST(RemoteMarshallingTest, SymbolSerialization) {
   clangd::Symbol Sym;
 
   auto ID = SymbolID::fromStr("057557CEBF6E6B2D");
-  EXPECT_TRUE(bool(ID));
+  ASSERT_TRUE(bool(ID));
   Sym.ID = *ID;
 
   index::SymbolInfo Info;
@@ -140,9 +136,9 @@
   // Check that symbols are exactly the same if the path to indexed project is
   // the same on indexing machine and the client.
   auto Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
   EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
   // Serialized paths are relative and have UNIX slashes.
   EXPECT_EQ(convert_to_slash(Serialized->definition().file_path(),
@@ -154,44 +150,39 @@
   // Missing definition is OK.
   Sym.Definition = clangd::SymbolLocation();
   Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_TRUE(Serialized);
-  Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Serialized);
+  EXPECT_TRUE(ProtobufMarshaller.fromProtobuf(*Serialized));
 
   // Relative path is absolute.
   *Serialized->mutable_canonical_declaration()->mutable_file_path() =
   convert_to_slash("/path/to/Declaration.h");
-  Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized));
 
   // Fail with an invalid URI.
   Location.FileURI = "Not A URI";
   Sym.Definition = Location;
-  Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(Sym));
 
   // Schemes other than "file" can not be used.
   auto UnittestURI = URI::create(testPath("home/SomePath.h"), "unittest");
-  EXPECT_TRUE(bool(UnittestURI));
+  ASSERT_TRUE(bool(UnittestURI));
   Location.FileURI = Strings.save(UnittestURI->toString()).begin();
   Sym.Definition = Location;
-  Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(Sym));
 
   // Passing root that is not prefix of the original file path.
   Location.FileURI = testPathU

[PATCH] D84535: [clangd] Switch from EXPECT_TRUE to ASSERT_TRUE in remote marshalling tests

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:64
   *Serialized->mutable_location()->mutable_file_path() = std::string();
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_FALSE(Deserialized);

kadircet wrote:
> nit: can be directly asserted
> 
> 
Do you mean "directly checked (whether via `EXPECTED` or `ASSERT`, I guess 
`EXPECTED` should be still better here, right?)" here and elsewhere? If so, 
done!



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:159
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_TRUE(Deserialized);
 

kadircet wrote:
> maybe make this an assert too (and also consider issuing ASSERT_FALSE 
> directly on the expression instead of an extra assingment)
Why is would asserting be better here? If I'm not dereferencing it I guess this 
should be worse because only the first assertion failure is checked, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84535



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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Instead of just mutex members we also consider mutex globals.
Unsurprisingly they are always in scope. Now the paper says that

  The scope of a class member is assumed to be its enclosing class,
  while the scope of a global variable is the translation unit in
  which it is defined.

But I don't think we should limit this to TUs where a definition is
available - a declaration is enough to acquire the mutex, and if a mutex
is really limited in scope to a translation unit, it should probably be
only declared there.

Fixes PR46354.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84604

Files:
  clang/lib/Analysis/ThreadSafety.cpp
  clang/test/SemaCXX/warn-thread-safety-analysis.cpp
  clang/test/SemaCXX/warn-thread-safety-negative.cpp


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,23 @@
 
 }  // end namespace SimpleTest
 
+namespace ScopeTest {
+
+Mutex globalMutex;
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+
+namespace ns {
+  Mutex namespaceMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!namespaceMutex);
+}
+
+void testGlobals() {
+  f(); // expected-warning {{calling function 'f' requires holding 
negative capability '!globalMutex'}}
+  ns::f(); // expected-warning {{calling function 'f' requires holding 
negative capability '!namespaceMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) 
__attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability(!FlightControl))) {
   const char *msg;
 
   while ((msg = deque_log_msg())) {
@@ -5044,13 +5045,13 @@
   }
 }
 
-void spawn_fake_logger_thread(void) {
+void spawn_fake_logger_thread(void) 
__attribute__((requires_capability(!FlightControl))) {
   acquire(Logger);
   logger_entry();
   release(Logger);
 }
 
-int main(void) {
+int main(void) __attribute__((requires_capability(!FlightControl))) {
   spawn_fake_flight_control_thread();
   spawn_fake_logger_thread();
 
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1266,13 +1266,22 @@
 }
 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
-  if (!CurrentMethod)
+  const threadSafety::til::SExpr *SExp = CapE.sexpr();
+  assert(SExp && "Null expressions should be ignored");
+
+  // Global variables are always in scope.
+  if (isa(SExp))
+return true;
+
+  // Members are in scope from methods of the same class.
+  if (const auto *P = dyn_cast(SExp)) {
+if (!CurrentMethod)
   return false;
-  if (const auto *P = dyn_cast_or_null(CapE.sexpr())) {
 const auto *VD = P->clangDecl();
 if (VD)
   return VD->getDeclContext() == CurrentMethod->getDeclContext();
   }
+
   return false;
 }
 


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -81,6 +81,23 @@
 
 }  // end namespace SimpleTest
 
+namespace ScopeTest {
+
+Mutex globalMutex;
+void f() EXCLUSIVE_LOCKS_REQUIRED(!globalMutex);
+
+namespace ns {
+  Mutex namespaceMutex;
+  void f() EXCLUSIVE_LOCKS_REQUIRED(!namespaceMutex);
+}
+
+void testGlobals() {
+  f(); // expected-warning {{calling function 'f' requires holding negative capability '!globalMutex'}}
+  ns::f(); // expected-warning {{calling function 'f' requires holding negative capability '!namespaceMutex'}}
+}
+
+}  // end namespace ScopeTest
+
 namespace DoubleAttribute {
 
 struct Foo {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5036,7 +5036,8 @@
 }
 
 extern const char *deque_log_msg(void) __attribute__((requires_capability(Logger)));
-void logger_entry(void) __attribute__((requires_capability(Logger))) {
+void logger_entry(void) __attribute__((requires_capability(Logger)))
+__attribute__((requires_capability

[PATCH] D84535: [clangd] Switch from EXPECT_TRUE to ASSERT_TRUE in remote marshalling tests

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 280743.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.

Address most comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84535

Files:
  clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
@@ -49,11 +49,11 @@
   "clangd/unittests/remote/MarshallingTests.cpp",
   Strings);
   auto Serialized = ProtobufMarshaller.toProtobuf(Original);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   EXPECT_EQ(Serialized->location().file_path(),
 "clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp");
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
   EXPECT_STREQ(Deserialized->Location.FileURI,
testPathURI("home/my-projects/llvm-project/clang-tools-extra/"
"clangd/unittests/remote/MarshallingTests.cpp",
@@ -61,38 +61,34 @@
 
   // Can't have empty paths.
   *Serialized->mutable_location()->mutable_file_path() = std::string();
-  Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized));
 
   clangd::Ref WithInvalidURI;
   // Invalid URI results in serialization failure.
   WithInvalidURI.Location.FileURI = "This is not a URI";
-  Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(WithInvalidURI));
 
   // Can not use URIs with scheme different from "file".
   auto UnittestURI =
   URI::create(testPath("project/lib/HelloWorld.cpp"), "unittest");
-  EXPECT_TRUE(bool(UnittestURI));
+  ASSERT_TRUE(bool(UnittestURI));
   WithInvalidURI.Location.FileURI =
   Strings.save(UnittestURI->toString()).begin();
-  Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(WithInvalidURI));
 
   // Paths transmitted over the wire can not be absolute, they have to be
   // relative.
   Ref WithAbsolutePath;
   *WithAbsolutePath.mutable_location()->mutable_file_path() =
   "/usr/local/user/home/HelloWorld.cpp";
-  Deserialized = ProtobufMarshaller.fromProtobuf(WithAbsolutePath);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(WithAbsolutePath));
 }
 
 TEST(RemoteMarshallingTest, SymbolSerialization) {
   clangd::Symbol Sym;
 
   auto ID = SymbolID::fromStr("057557CEBF6E6B2D");
-  EXPECT_TRUE(bool(ID));
+  ASSERT_TRUE(bool(ID));
   Sym.ID = *ID;
 
   index::SymbolInfo Info;
@@ -140,9 +136,9 @@
   // Check that symbols are exactly the same if the path to indexed project is
   // the same on indexing machine and the client.
   auto Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_TRUE(Deserialized);
+  ASSERT_TRUE(Deserialized);
   EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
   // Serialized paths are relative and have UNIX slashes.
   EXPECT_EQ(convert_to_slash(Serialized->definition().file_path(),
@@ -154,44 +150,40 @@
   // Missing definition is OK.
   Sym.Definition = clangd::SymbolLocation();
   Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_TRUE(Deserialized);
 
   // Relative path is absolute.
   *Serialized->mutable_canonical_declaration()->mutable_file_path() =
   convert_to_slash("/path/to/Declaration.h");
-  Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized));
 
   // Fail with an invalid URI.
   Location.FileURI = "Not A URI";
   Sym.Definition = Location;
-  Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(Sym));
 
   // Schemes other than "file" can not be used.
   auto UnittestURI = URI::create(testPath("home/SomePath.h"), "unittest");
-  EXPECT_TRUE(bool(UnittestURI));
+  ASSERT_TRUE(bool(UnittestURI));
   Location.FileURI = Strings.save(UnittestURI->toString()).begin();
   Sym.Definition = Location;
-  Serialized = ProtobufMarshaller.toProtobuf(Sym);
-  EXPECT_FALSE(Serialized);
+  EXPECT_FALSE(ProtobufMarshaller.toProtobuf(Sym));
 
   // Passing root that is not prefix of the original file path.
   Location.FileURI = testPathURI("home/File.h", Strings);
   Sym.Definition = Loc

[PATCH] D84603: Thread safety analysis: More consistent warning message

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Other warning messages for negative capabilities also mention their
kind, and the double space was ugly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84603

Files:
  clang/lib/Analysis/ThreadSafety.cpp
  clang/test/SemaCXX/warn-thread-safety-analysis.cpp
  clang/test/SemaCXX/warn-thread-safety-negative.cpp


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -50,7 +50,7 @@
   }
 
   void bar() {
-baz();// expected-warning {{calling function 'baz' requires 
holding  '!mu'}}
+baz();// expected-warning {{calling function 'baz' requires 
holding negative capability '!mu'}}
   }
 
   void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4985,7 +4985,7 @@
   }
 
   void bar() {
-bar2();   // expected-warning {{calling function 'bar2' requires 
holding  '!mu'}}
+bar2();   // expected-warning {{calling function 'bar2' requires 
holding negative capability '!mu'}}
   }
 
   void bar2() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1641,8 +1641,8 @@
 // Otherwise the negative requirement must be propagated to the caller.
 LDat = FSet.findLock(Analyzer->FactMan, Cp);
 if (!LDat) {
-  Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
-   LK_Shared, Loc);
+  Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+   Cp.toString(), LK_Shared, Loc);
 }
 return;
   }


Index: clang/test/SemaCXX/warn-thread-safety-negative.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -50,7 +50,7 @@
   }
 
   void bar() {
-baz();// expected-warning {{calling function 'baz' requires holding  '!mu'}}
+baz();// expected-warning {{calling function 'baz' requires holding negative capability '!mu'}}
   }
 
   void baz() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4985,7 +4985,7 @@
   }
 
   void bar() {
-bar2();   // expected-warning {{calling function 'bar2' requires holding  '!mu'}}
+bar2();   // expected-warning {{calling function 'bar2' requires holding negative capability '!mu'}}
   }
 
   void bar2() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -1641,8 +1641,8 @@
 // Otherwise the negative requirement must be propagated to the caller.
 LDat = FSet.findLock(Analyzer->FactMan, Cp);
 if (!LDat) {
-  Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(),
-   LK_Shared, Loc);
+  Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK,
+   Cp.toString(), LK_Shared, Loc);
 }
 return;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84602: [MSP430] Expose msp430_builtin calling convention to C code

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko created this revision.
atrosinenko added reviewers: asl, aaron.ballman, rjmccall.
Herald added subscribers: llvm-commits, hiraditya.
Herald added projects: clang, LLVM.

According to MSP430 EABI document 
, Section 6.3, some of 
compiler helper functions require a special calling convention that is used for 
some of LibCalls accepting two 64-bit arguments.

As part of porting the compiler-rt builtins library to MSP430 I need to not 
only internally emit calls to these LibCalls but also to explicitly declare 
such functions in C code (for explicilty calling them from the unit test code) 
and to implement them in C (when I do not want to replace the generic C 
implementation with custom assembler code).

References: How to add an attribute 
 
chapter covers basics of adding a new attribute (not specifically calling 
convention-related).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84602

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/Specifiers.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Basic/Targets/MSP430.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/msp430-cc-builtin.c
  clang/test/Sema/attr-msp430.c
  clang/tools/libclang/CXType.cpp
  llvm/lib/Target/MSP430/MSP430ISelLowering.cpp

Index: llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
===
--- llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
+++ llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
@@ -573,6 +573,7 @@
 report_fatal_error("Unsupported calling convention");
   case CallingConv::C:
   case CallingConv::Fast:
+  case CallingConv::MSP430_BUILTIN:
 return LowerCCCArguments(Chain, CallConv, isVarArg, Ins, dl, DAG, InVals);
   case CallingConv::MSP430_INTR:
 if (Ins.empty())
Index: clang/tools/libclang/CXType.cpp
===
--- clang/tools/libclang/CXType.cpp
+++ clang/tools/libclang/CXType.cpp
@@ -666,6 +666,7 @@
   TCALLINGCONV(Swift);
   TCALLINGCONV(PreserveMost);
   TCALLINGCONV(PreserveAll);
+case CC_MSP430Builtin: return CXCallingConv_Unexposed;
 case CC_SpirFunction: return CXCallingConv_Unexposed;
 case CC_OpenCLKernel: return CXCallingConv_Unexposed;
   break;
Index: clang/test/Sema/attr-msp430.c
===
--- clang/test/Sema/attr-msp430.c
+++ clang/test/Sema/attr-msp430.c
@@ -11,3 +11,6 @@
 
 __attribute__((interrupt(0))) void f6(void);
 __attribute__((interrupt(63))) void f7(void);
+
+__attribute__((msp430_builtin)) int t2; // expected-warning {{'msp430_builtin' only applies to function types; type here is 'int'}}
+__attribute__((msp430_builtin)) void f8(long long a, long long b);
Index: clang/test/CodeGen/msp430-cc-builtin.c
===
--- /dev/null
+++ clang/test/CodeGen/msp430-cc-builtin.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -triple msp430 -emit-llvm < %s | FileCheck %s
+
+__attribute__((msp430_builtin)) int f(long long x, long long y);
+
+__attribute__((msp430_builtin)) int g(long long x, long long y) {
+  return (int)42;
+}
+// CHECK: define cc94 {{(dso_local )?}}i16 @g(i64 %x, i64 %y)
+
+int caller()
+{
+  return f(0, 0);
+// CHECK: call cc94 i16 @f
+}
+
+// CHECK: declare cc94 {{(dso_local )?}}i16 @f(i64, i64)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -124,7 +124,8 @@
   case ParsedAttr::AT_Pcs: \
   case ParsedAttr::AT_IntelOclBicc:\
   case ParsedAttr::AT_PreserveMost:\
-  case ParsedAttr::AT_PreserveAll
+  case ParsedAttr::AT_PreserveAll: \
+  case ParsedAttr::AT_MSP430Builtin
 
 // Function type attributes.
 #define FUNCTION_TYPE_ATTRS_CASELIST   \
@@ -7248,6 +7249,8 @@
 return createSimpleAttr(Ctx, Attr);
   case ParsedAttr::AT_PreserveAll:
 return createSimpleAttr(Ctx, Attr);
+  case ParsedAttr::AT_MSP430Builtin:
+return createSimpleAttr(Ctx, Attr);
   }
   llvm_unreachable("unexpected attribute kind!");
 }
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -4459,6 +4459,9 @@
   case ParsedAttr::AT_PreserveAll:
 D->addAttr(::new (S.Context) PreserveAllAttr(S.Context, AL));
 return;
+  case ParsedAttr::AT_MSP430Builtin:
+D-

[PATCH] D84591: [clang-tidy][NFC] Replace comment by private method

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D84591#2174473 , @hanneskaeufler 
wrote:

> In D84591#2174447 , @aaron.ballman 
> wrote:
>
> > In D84591#2174442 , 
> > @hanneskaeufler wrote:
> >
> > > In D84591#2174437 , 
> > > @aaron.ballman wrote:
> > >
> > > > LGTM!
> > >
> > >
> > > Awesome, thanks!
> >
> >
> > Do you need someone to commit on your behalf? If so, can you give me the 
> > name and email address you'd like me to attribute the commit to (I've got 
> > `Hannes Käufler `)?
>
>
> Uh, yeah, I don't suppose I have commit rights. First time contributor here 
> :) Name and E-Mail is correct!


Thank you for the patch! I've committed on your behalf in 
3bbf3e026d3c692966583075ae6d12c4575e9d72 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84591



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

[clang-tools-extra] 3bbf3e0 - Replace comment by private method; NFC.

[Aaron Ballman via cfe-commits]

Author: Hannes Käufler
Date: 2020-07-26T13:59:45-04:00
New Revision: 3bbf3e026d3c692966583075ae6d12c4575e9d72

URL: 
https://github.com/llvm/llvm-project/commit/3bbf3e026d3c692966583075ae6d12c4575e9d72
DIFF: 
https://github.com/llvm/llvm-project/commit/3bbf3e026d3c692966583075ae6d12c4575e9d72.diff

LOG: Replace comment by private method; NFC.

Added: 


Modified: 
clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp 
b/clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
index 68bb987c1275..1cae618dfd09 100644
--- a/clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
+++ b/clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
@@ -123,12 +123,7 @@ class HeaderGuardPPCallbacks : public PPCallbacks {
 
 // Emit warnings for headers that are missing guards.
 checkGuardlessHeaders();
-
-// Clear all state.
-Macros.clear();
-Files.clear();
-Ifndefs.clear();
-EndIfs.clear();
+clearAllState();
   }
 
   bool wouldFixEndifComment(StringRef FileName, SourceLocation EndIf,
@@ -255,6 +250,13 @@ class HeaderGuardPPCallbacks : public PPCallbacks {
   }
 
 private:
+  void clearAllState() {
+Macros.clear();
+Files.clear();
+Ifndefs.clear();
+EndIfs.clear();
+  }
+
   std::vector> Macros;
   llvm::StringMap Files;
   std::map>



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

[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar created this revision.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso, 
dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, 
baloghadamsoftware, xazax.hun.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84600

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp
  clang/test/Analysis/smart-ptr.cpp

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -12,7 +12,7 @@
   std::unique_ptr Q = std::move(P);
   if (Q)
 clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
-  *Q.get() = 1; // no-warning
+  *Q.get() = 1; // no-warning
   if (P)
 clang_analyzer_warnIfReached(); // no-warning
   // TODO: Report a null dereference (instead).
@@ -50,37 +50,38 @@
 
 void derefAfterDefaultCtr() {
   std::unique_ptr P;
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterCtrWithNull() {
   std::unique_ptr P(nullptr);
-  *P; // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  *P; // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
-void derefAfterCtrWithNullReturnMethod() {
-  std::unique_ptr P(return_null());
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+void derefAfterCtrWithNullVariable() {
+  A *InnerPtr = nullptr;
+  std::unique_ptr P(InnerPtr);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterRelease() {
   std::unique_ptr P(new A());
   P.release();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterReset() {
   std::unique_ptr P(new A());
   P.reset();
   clang_analyzer_numTimesReached(); // expected-warning {{1}}
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNull() {
   std::unique_ptr P(new A());
   P.reset(nullptr);
-  P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterResetWithNonNull() {
@@ -102,6 +103,12 @@
   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
 }
 
+void derefOnReleasedValidRawPtr() {
+  std::unique_ptr P(new A());
+  A *AP = P.release();
+  AP->foo(); // No warning.
+}
+
 void pass_smart_ptr_by_ref(std::unique_ptr &a);
 void pass_smart_ptr_by_const_ref(const std::unique_ptr &a);
 void pass_smart_ptr_by_rvalue_ref(std::unique_ptr &&a);
@@ -118,7 +125,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ref(P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -128,7 +135,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
   }
   {
 std::unique_ptr P;
@@ -138,7 +145,7 @@
   {
 std::unique_ptr P;
 pass_smart_ptr_by_const_ptr(&P);
-P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
   }
 }
 
@@ -162,7 +169,7 @@
   {
 StructWithSmartPtr S;
 pass_struct_with_smart_ptr_by_const_ref(S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer [alpha.cplusplus.SmartPtr]}}
+S.P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [alpha.cplusplus.SmartPtr]}}
   }
   {
 StructWithSmartPtr S;
@@ -

[PATCH] D84591: [clang-tidy][NFC] Replace comment by private method

[Hannes Käufler via Phabricator via cfe-commits]

hanneskaeufler added a comment.

In D84591#2174447 , @aaron.ballman 
wrote:

> In D84591#2174442 , @hanneskaeufler 
> wrote:
>
> > In D84591#2174437 , @aaron.ballman 
> > wrote:
> >
> > > LGTM!
> >
> >
> > Awesome, thanks!
>
>
> Do you need someone to commit on your behalf? If so, can you give me the name 
> and email address you'd like me to attribute the commit to (I've got `Hannes 
> Käufler `)?


Uh, yeah, I don't suppose I have commit rights. First time contributor here :) 
Name and E-Mail is correct!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84591



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

[PATCH] D84599: [clang-index] Use NamedDecl::getDeclName() instead of NamedDecl::printName in USRGenerator::EmitDeclName

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 280734.
riccibruno added a comment.

Update a comment I originally missed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84599

Files:
  clang/lib/Index/USRGeneration.cpp


Index: clang/lib/Index/USRGeneration.cpp
===
--- clang/lib/Index/USRGeneration.cpp
+++ clang/lib/Index/USRGeneration.cpp
@@ -167,8 +167,7 @@
   void VisitTemplateName(TemplateName Name);
   void VisitTemplateArgument(const TemplateArgument &Arg);
 
-  /// Emit a Decl's name using NamedDecl::printName() and return true if
-  ///  the decl had no name.
+  /// Emit a Decl's name and return true if the decl had no name.
   bool EmitDeclName(const NamedDecl *D);
 };
 } // end anonymous namespace
@@ -179,7 +178,7 @@
 
 bool USRGenerator::EmitDeclName(const NamedDecl *D) {
   const unsigned startSize = Buf.size();
-  D->printName(Out);
+  Out << D->getDeclName();
   const unsigned endSize = Buf.size();
   return startSize == endSize;
 }


Index: clang/lib/Index/USRGeneration.cpp
===
--- clang/lib/Index/USRGeneration.cpp
+++ clang/lib/Index/USRGeneration.cpp
@@ -167,8 +167,7 @@
   void VisitTemplateName(TemplateName Name);
   void VisitTemplateArgument(const TemplateArgument &Arg);
 
-  /// Emit a Decl's name using NamedDecl::printName() and return true if
-  ///  the decl had no name.
+  /// Emit a Decl's name and return true if the decl had no name.
   bool EmitDeclName(const NamedDecl *D);
 };
 } // end anonymous namespace
@@ -179,7 +178,7 @@
 
 bool USRGenerator::EmitDeclName(const NamedDecl *D) {
   const unsigned startSize = Buf.size();
-  D->printName(Out);
+  Out << D->getDeclName();
   const unsigned endSize = Buf.size();
   return startSize == endSize;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84599: [clang-index] Use NamedDecl::getDeclName() instead of NamedDecl::printName in USRGenerator::EmitDeclName

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: akyrtzi, jkorous, sammccall, hokein.
riccibruno added a project: clang.
Herald added subscribers: cfe-commits, arphaman, dexonsmith.

`NamedDecl::printName` is used as a customisation point for 
`getNameForDiagnostic`. The default implementation is just sending the 
`DeclarationName` into the output stream, but it can be overloaded to display a 
more user-friendly name for some entities.

Currently only `DecompositionDecl` and `MSGuidDecl` have an overload (and they 
were both added after this code was written) but I will be adding overloads to 
`VarDecl`, `FieldDecl`, `EnumDecl` and `RecordDecl` to better handle unnamed 
entities.

To preserve the current behaviour (that is except for `DecompositionDecl` and 
`MSGuidDecl` which I don't think are relevant here?), just send the 
`DeclarationName` into the output instead of going through 
`NamedDecl::printName`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84599

Files:
  clang/lib/Index/USRGeneration.cpp


Index: clang/lib/Index/USRGeneration.cpp
===
--- clang/lib/Index/USRGeneration.cpp
+++ clang/lib/Index/USRGeneration.cpp
@@ -179,7 +179,7 @@
 
 bool USRGenerator::EmitDeclName(const NamedDecl *D) {
   const unsigned startSize = Buf.size();
-  D->printName(Out);
+  Out << D->getDeclName();
   const unsigned endSize = Buf.size();
   return startSize == endSize;
 }


Index: clang/lib/Index/USRGeneration.cpp
===
--- clang/lib/Index/USRGeneration.cpp
+++ clang/lib/Index/USRGeneration.cpp
@@ -179,7 +179,7 @@
 
 bool USRGenerator::EmitDeclName(const NamedDecl *D) {
   const unsigned startSize = Buf.size();
-  D->printName(Out);
+  Out << D->getDeclName();
   const unsigned endSize = Buf.size();
   return startSize == endSize;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] a2f83d5 - [clang][NFC] Add tests for the use of NamedDecl::getDeclName in the unused/unneeded diagnostics.

[Bruno Ricci via cfe-commits]

Author: Bruno Ricci
Date: 2020-07-26T17:20:56+01:00
New Revision: a2f83d5a07daf7a9b717fff8916c44cd7cc1c678

URL: 
https://github.com/llvm/llvm-project/commit/a2f83d5a07daf7a9b717fff8916c44cd7cc1c678
DIFF: 
https://github.com/llvm/llvm-project/commit/a2f83d5a07daf7a9b717fff8916c44cd7cc1c678.diff

LOG: [clang][NFC] Add tests for the use of NamedDecl::getDeclName in the 
unused/unneeded diagnostics.

Added: 


Modified: 
clang/test/SemaCXX/warn-func-not-needed.cpp
clang/test/SemaCXX/warn-member-not-needed.cpp
clang/test/SemaCXX/warn-unused-filescoped.cpp
clang/test/SemaCXX/warn-variable-not-needed.cpp

Removed: 




diff  --git a/clang/test/SemaCXX/warn-func-not-needed.cpp 
b/clang/test/SemaCXX/warn-func-not-needed.cpp
index 65721f44f570..5040aaad9460 100644
--- a/clang/test/SemaCXX/warn-func-not-needed.cpp
+++ b/clang/test/SemaCXX/warn-func-not-needed.cpp
@@ -1,13 +1,23 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wall %s
 
 namespace test1 {
-  static void f() {} // expected-warning {{is not needed and will not be 
emitted}}
-  static void f();
-  template 
-  void foo() {
-f();
-  }
+static void f() {} // expected-warning {{function 'f' is not needed and will 
not be emitted}}
+static void f();
+template 
+void foo() {
+  f();
+}
+}
+
+namespace test1_template {
+template  static void f() {}
+template <> void f() {} // expected-warning {{function 'f' is not needed 
and will not be emitted}}
+template 
+void foo() {
+  f();
+  f();
 }
+} // namespace test1_template
 
 namespace test2 {
   static void f() {}

diff  --git a/clang/test/SemaCXX/warn-member-not-needed.cpp 
b/clang/test/SemaCXX/warn-member-not-needed.cpp
index 61bb3488c611..95241f4f7fee 100644
--- a/clang/test/SemaCXX/warn-member-not-needed.cpp
+++ b/clang/test/SemaCXX/warn-member-not-needed.cpp
@@ -1,11 +1,19 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wunneeded-member-function %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunneeded-member-function 
-Wno-unused-template %s
 
 namespace {
   class A {
-void g() {} // expected-warning {{is not needed and will not be emitted}}
+void g() {} // expected-warning {{member function 'g' is not needed and 
will not be emitted}}
+template  void gt(T) {}
+template <> void gt(int) {} // expected-warning {{member function 
'gt' is not needed and will not be emitted}}
+template <> void gt(float) {}// expected-warning {{member function 
'gt' is not needed and will not be emitted}}
+
 template 
 void foo() {
   g();
+  gt(0);
+  gt(0.0f);
+  gt(0.0);
 }
   };
+  template void A::gt(double); // no-warning
 }

diff  --git a/clang/test/SemaCXX/warn-unused-filescoped.cpp 
b/clang/test/SemaCXX/warn-unused-filescoped.cpp
index 7ea398feb2b1..056543d5eeb0 100644
--- a/clang/test/SemaCXX/warn-unused-filescoped.cpp
+++ b/clang/test/SemaCXX/warn-unused-filescoped.cpp
@@ -1,14 +1,15 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-template 
-Wunused-member-function -Wno-unused-local-typedefs -Wno-c++11-extensions 
-std=c++98 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-template 
-Wunused-member-function -Wno-unused-local-typedefs \
+// RUN:-Wno-c++11-extensions -Wno-c++14-extensions -std=c++98 %s
 // RUN: %clang_cc1 -fsyntax-only -verify -Wunused -Wunused-template 
-Wunused-member-function -Wno-unused-local-typedefs -std=c++14 %s
 
 #ifdef HEADER
 
-static void headerstatic() {}  // expected-warning{{unused}}
+static void headerstatic() {} // expected-warning{{unused function 
'headerstatic'}}
 static inline void headerstaticinline() {}
 
 namespace {
-  void headeranon() {}  // expected-warning{{unused}}
-  inline void headerinlineanon() {}
+void headeranon() {} // expected-warning{{unused function 'headeranon'}}
+inline void headerinlineanon() {}
 }
 
 namespace test7
@@ -43,31 +44,31 @@ namespace pr19713 {
 #define HEADER
 #include "warn-unused-filescoped.cpp"
 
-static void f1(); // expected-warning{{unused}}
+static void f1(); // expected-warning{{unused function 'f1'}}
 
 namespace {
-  void f2();  // expected-warning{{unused}}
+void f2(); // expected-warning{{unused function 'f2'}}
 
-  void f3() { }  // expected-warning{{unused}}
+void f3() {} // expected-warning{{unused function 'f3'}}
 
-  struct S {
-void m1() { }  // expected-warning{{unused}}
-void m2();  // expected-warning{{unused}}
-void m3();
-S(const S&);
-void operator=(const S&);
-  };
+struct S {
+  void m1() {} // expected-warning{{unused member function 'm1'}}
+  void m2();   // expected-warning{{unused member function 'm2'}}
+  void m3();
+  S(const S &);
+  void operator=(const S &);
+};
 
   template 
   struct TS {
 void m();
   };
-  template <> void TS::m() { }  // expected-warning{{unused}}
+  template <> void TS::m() {} // expected-warning{{unused member function 
'm'}}
 
   template 
-  void tf() { }  // expected-w

[clang] b0512ee - [clang][NFC] Add a test for __attribute__((flag_enum)) with an unnamed enumeration.

[Bruno Ricci via cfe-commits]

Author: Bruno Ricci
Date: 2020-07-26T17:24:43+01:00
New Revision: b0512eed1e9dc03dba4ef8cccee73c13d3487565

URL: 
https://github.com/llvm/llvm-project/commit/b0512eed1e9dc03dba4ef8cccee73c13d3487565
DIFF: 
https://github.com/llvm/llvm-project/commit/b0512eed1e9dc03dba4ef8cccee73c13d3487565.diff

LOG: [clang][NFC] Add a test for __attribute__((flag_enum)) with an unnamed 
enumeration.

Added: 


Modified: 
clang/test/Sema/attr-flag-enum.c

Removed: 




diff  --git a/clang/test/Sema/attr-flag-enum.c 
b/clang/test/Sema/attr-flag-enum.c
index ae3e3ad5ab99..467afd950973 100644
--- a/clang/test/Sema/attr-flag-enum.c
+++ b/clang/test/Sema/attr-flag-enum.c
@@ -6,6 +6,10 @@ enum __attribute__((flag_enum)) flag {
   ec = 0x8,
 };
 
+enum __attribute__((flag_enum)) {
+  g = 0x7,  // expected-warning {{enumeration value 'g' is out of range of 
flags in enumeration type ''}}
+};
+
 enum __attribute__((flag_enum)) flag2 {
   ga = 0x1,
   gb = 0x4,



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

[clang] ca9bfc2 - [clang][NFC] Remove spurious +x flag on SemaConcept.cpp

[Bruno Ricci via cfe-commits]

Author: Bruno Ricci
Date: 2020-07-26T17:10:59+01:00
New Revision: ca9bfc20f48c82a9f223ec814697714a16d1a22d

URL: 
https://github.com/llvm/llvm-project/commit/ca9bfc20f48c82a9f223ec814697714a16d1a22d
DIFF: 
https://github.com/llvm/llvm-project/commit/ca9bfc20f48c82a9f223ec814697714a16d1a22d.diff

LOG: [clang][NFC] Remove spurious +x flag on SemaConcept.cpp

Added: 


Modified: 
clang/lib/Sema/SemaConcept.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
old mode 100755
new mode 100644



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

[PATCH] D84306: [clang-format][NFC] Be more careful about the layout of FormatToken.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 280730.
riccibruno added a comment.

Use `is` and `isNot`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84306

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/WhitespaceManager.cpp

Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -49,7 +49,7 @@
   bool IsAligned, bool InPPDirective) {
   if (Tok.Finalized)
 return;
-  Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
+  Tok.setDecision((Newlines > 0) ? FD_Break : FD_Continue);
   Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
Spaces, StartOfTokenColumn, Newlines, "", "",
IsAligned, InPPDirective && !Tok.IsFirst,
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -472,19 +472,19 @@
   // individual members in a type member list, which would normally
   // trigger BK_Block. In both cases, this must be parsed as an inline
   // braced init.
-  Tok->BlockKind = BK_BracedInit;
+  Tok->setBlockKind(BK_BracedInit);
 else if (PrevTok->is(tok::r_paren))
   // `) { }` can only occur in function or method declarations in JS.
-  Tok->BlockKind = BK_Block;
+  Tok->setBlockKind(BK_Block);
   } else {
-Tok->BlockKind = BK_Unknown;
+Tok->setBlockKind(BK_Unknown);
   }
   LBraceStack.push_back(Tok);
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
 break;
-  if (LBraceStack.back()->BlockKind == BK_Unknown) {
+  if (LBraceStack.back()->is(BK_Unknown)) {
 bool ProbablyBracedList = false;
 if (Style.Language == FormatStyle::LK_Proto) {
   ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
@@ -524,11 +524,11 @@
   }
 }
 if (ProbablyBracedList) {
-  Tok->BlockKind = BK_BracedInit;
-  LBraceStack.back()->BlockKind = BK_BracedInit;
+  Tok->setBlockKind(BK_BracedInit);
+  LBraceStack.back()->setBlockKind(BK_BracedInit);
 } else {
-  Tok->BlockKind = BK_Block;
-  LBraceStack.back()->BlockKind = BK_Block;
+  Tok->setBlockKind(BK_Block);
+  LBraceStack.back()->setBlockKind(BK_Block);
 }
   }
   LBraceStack.pop_back();
@@ -545,8 +545,8 @@
 case tok::kw_switch:
 case tok::kw_try:
 case tok::kw___try:
-  if (!LBraceStack.empty() && LBraceStack.back()->BlockKind == BK_Unknown)
-LBraceStack.back()->BlockKind = BK_Block;
+  if (!LBraceStack.empty() && LBraceStack.back()->is(BK_Unknown))
+LBraceStack.back()->setBlockKind(BK_Block);
   break;
 default:
   break;
@@ -557,8 +557,8 @@
 
   // Assume other blocks for all unclosed opening braces.
   for (unsigned i = 0, e = LBraceStack.size(); i != e; ++i) {
-if (LBraceStack[i]->BlockKind == BK_Unknown)
-  LBraceStack[i]->BlockKind = BK_Block;
+if (LBraceStack[i]->is(BK_Unknown))
+  LBraceStack[i]->setBlockKind(BK_Block);
   }
 
   FormatTok = Tokens->setPosition(StoredPosition);
@@ -584,7 +584,7 @@
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
  "'{' or macro block token expected");
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
-  FormatTok->BlockKind = BK_Block;
+  FormatTok->setBlockKind(BK_Block);
 
   size_t PPStartHash = computePPHash();
 
@@ -614,7 +614,7 @@
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
  : !FormatTok->is(tok::r_brace)) {
 Line->Level = InitialLevel;
-FormatTok->BlockKind = BK_Block;
+FormatTok->setBlockKind(BK_Block);
 return;
   }
 
@@ -690,7 +690,7 @@
 }
 
 void UnwrappedLineParser::parseChildBlock() {
-  FormatTok->BlockKind = BK_Block;
+  FormatTok->setBlockKind(BK_Block);
   nextToken();
   {
 bool SkipIndent = (Style.Language == FormatStyle::LK_JavaScript &&
@@ -1476,7 +1476,7 @@
 // C# needs this change to ensure that array initialisers and object
 // initialisers are indented the same way.
 if (Style.isCSharp())
-  FormatTok->BlockKind = BK_BracedInit;
+  FormatTok->setBlockKind(BK_BracedInit);
 nextToken();
 parseBracedList();
   } else if (Style.Language == FormatStyle::LK_Proto &&
@@ -1747,10 +1747,10 @@
 }
 
 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D84005#2172747 , @doug.gregor wrote:

> In D84005#2171982 , @MForster wrote:
>
> > @milseman, @doug.gregor, could you please help with the open questions on 
> > this review?
> >
> > Specifically:
> >
> > - Is `ns_error_domain` ever needed for something other than an enum?
>
>
> No, it only makes sense on enums.
>
> > - Why is this designed in the way it is (requiring an identifier for the 
> > domain, not allowing literals and then only using the name of the 
> > identifier from Swift)?
>
> It's codifying the design of NSError, which has been this way since... longer 
> than Clang has existed, and is independent of Swift. NSErrors have a domain 
> (identified by an NSString constant symbol, not a literal, for pointer 
> uniqueness and code size)  and a code (an integer, conventionally defined by 
> an enum). The two need to be used together, e.g., you create an NSError with 
> a domain and a code from that domain. This attribute finally ties those 
> things together at the source level.


Ah, thank you, that makes the design far more clear to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84591: [clang-tidy][NFC] Replace comment by private method

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D84591#2174442 , @hanneskaeufler 
wrote:

> In D84591#2174437 , @aaron.ballman 
> wrote:
>
> > LGTM!
>
>
> Awesome, thanks!


Do you need someone to commit on your behalf? If so, can you give me the name 
and email address you'd like me to attribute the commit to (I've got `Hannes 
Käufler `)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84591



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

[PATCH] D84591: [clang-tidy][NFC] Replace comment by private method

[Hannes Käufler via Phabricator via cfe-commits]

hanneskaeufler added a comment.

In D84591#2174437 , @aaron.ballman 
wrote:

> LGTM!


Awesome, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84591



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

[PATCH] D84591: [clang-tidy][NFC] Replace comment by private method

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84591



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

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D83174#2174294 , @gargvaibhav64 
wrote:

> Hi everyone, are there any more changes required on this review?


FWIW, I'm holding my LG until there's a test case which covers the changes.


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

https://reviews.llvm.org/D83174



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

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

[David Chisnall via Phabricator via cfe-commits]

theraven added a comment.
Herald added a reviewer: aaron.ballman.
Herald added a reviewer: jdoerfert.

Sorry for getting to this late, I assumed it was a runtime-agnostic feature 
until someone filed a bug against the GNUstep runtime saying that we didn't 
implement it.  It would be polite to tag me in reviews for features that 
contain runtime-specific things.

The current implementation doesn't look as if it's runtime-specific.  Is there 
a reason that it's in CGObjCMacCommon rather than in the generic CGObjCRuntime 
class?

Looking at the code, it appears that it is correct only for objects created 
with `+alloc`.  Consider the following example:

  #import 
  #include 
  
  @interface X : NSObject
  - (void)test __attribute__((objc_direct));
  @end
  
  @implementation X
  + (void)initialize
  {
printf("+initialize called on %p\n", self);
  }
  - (void)test
  {
printf("Test called on %p\n", self);
  }
  @end
  
  int main()
  {
@autoreleasepool
{
X *x = class_createInstance(objc_getClass("X"), 0);
[x test];
}
  }

I don't believe this does the right thing (unfortunately, the latest XCode 
requires Catalina and it breaks too many things for me to upgrade from Mojave, 
so I can't test it and see if there are any tweaks to the runtime).  The API 
guarantee for `class_createInstance` does not require it to call `+initialize`. 
 I can see a few ways of fixing this:

Either:

- Unconditionally do the `[self self]` dance on all direct methods (not ideal, 
since it's likely to offset the performance win) for runtimes without knowledge 
of direct functions and one out of:
  - Set a metadata flag on any class with direct methods to require 
`class_createInstance` (and any other allocation paths that I haven't thought 
of) in the runtime to call `+initialize` on instantiation.
  - Modify the API contract of `class_createInstance` to send `+initialize` 
unconditionally.
  - Provide an `objc_msgSendDirect` that takes the target function in place of 
the selector and checks that the receiver is initialise, teach the optimisers 
to inline through this in cases where they can prove that the receiver is 
already initialised.
- Document that direct methods do not trigger `+initialize`, generalise that to 
class methods and remove the existing `+self` dance.

For direct class methods, avoiding the `+self` dance might best avoided by 
using a construct like the C++ static initialiser for a pointer to the class at 
the call site (rather than a direct reference to the class), which guarantees 
that it's initialised in the callee and skipping this when the receiver is 
`self` in an method (`+initialize` is guaranteed to have been called on `self` 
on entry to any method in this case).  The pseudocode is something like (where 
`+direct` is a direct method):

The commit message for this refers to `objc_opt_self`.  This is presumably a 
fast-path for a `self` message send, but the only reference to that anywhere in 
the LLVM tree is in LLDB.  We don't appear to have any tests in clang for it 
and neither upstream nor Apple clang appear to generate calls for it, so I'm 
not sure what 'an appropriate deployment target' is in the commit message.


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

https://reviews.llvm.org/D69991



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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Stephen Neuendorffer via Phabricator via cfe-commits]

stephenneuendorffer added inline comments.



Comment at: mlir/examples/standalone/CMakeLists.txt:35
+endif()
+
 include(TableGen)

mehdi_amini wrote:
> mehdi_amini wrote:
> > I am a bit unsure that it is desirable that it is desirable to need this 
> > change?
> > This requires every single user of LLVM to do this. Is there a way this can 
> > be done in the call to `find_package(LLVM)` instead?
> (Doc for the usual way to integrate LLVM: 
> https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project )
I tend to agree with Mehdi.  I think stuff like this which is *required* to use 
LLVM should get pushed into what LLVM exports.  (Actually there is a bunch of 
other configuration stuff which also needs to get pushed into the export, 
otherwise configuration defaults are all wrong and things like that.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79219



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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/examples/standalone/CMakeLists.txt:35
+endif()
+
 include(TableGen)

mehdi_amini wrote:
> I am a bit unsure that it is desirable that it is desirable to need this 
> change?
> This requires every single user of LLVM to do this. Is there a way this can 
> be done in the call to `find_package(LLVM)` instead?
(Doc for the usual way to integrate LLVM: 
https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79219



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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added inline comments.



Comment at: mlir/examples/standalone/CMakeLists.txt:35
+endif()
+
 include(TableGen)

I am a bit unsure that it is desirable that it is desirable to need this change?
This requires every single user of LLVM to do this. Is there a way this can be 
done in the call to `find_package(LLVM)` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79219



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

[PATCH] D84535: [clangd] Switch from EXPECT_TRUE to ASSERT_TRUE in remote marshalling tests

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks LGTM!




Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:64
   *Serialized->mutable_location()->mutable_file_path() = std::string();
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_FALSE(Deserialized);

nit: can be directly asserted





Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:70
   WithInvalidURI.Location.FileURI = "This is not a URI";
   Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI);
   EXPECT_FALSE(Serialized);

nit: can be directly asserted





Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:79
   Strings.save(UnittestURI->toString()).begin();
   Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI);
   EXPECT_FALSE(Serialized);

nit: can be directly asserted



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:87
   "/usr/local/user/home/HelloWorld.cpp";
   Deserialized = ProtobufMarshaller.fromProtobuf(WithAbsolutePath);
   EXPECT_FALSE(Deserialized);

nit: again can be directly asserted



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:159
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_TRUE(Deserialized);
 

maybe make this an assert too (and also consider issuing ASSERT_FALSE directly 
on the expression instead of an extra assingment)



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:165
   Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   EXPECT_FALSE(Deserialized);
 

same as the previous one



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:171
   Serialized = ProtobufMarshaller.toProtobuf(Sym);
   EXPECT_FALSE(Serialized);
 

same as the previous one



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:193
   Marshaller WrongMarshaller(testPath("nothome/"), testPath("home/"));
   Serialized = WrongMarshaller.toProtobuf(Sym);
   EXPECT_FALSE(Serialized);

nit: maybe directly issue `EXPECT_FALSE(WrongMarshaller.toProtobuf(Sym))` 
instead of going through an extra variable.



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:275
 ValidHeaders.size());
-  EXPECT_TRUE(Serialized);
+  ASSERT_TRUE(Serialized);
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);

this should be done before the EXPECT_EQ above (which accesses `headers_size()`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84535



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

[PATCH] D84513: [clangd] Collect references for externally visible main-file symbols

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

In D84513#2172028 , @kadircet wrote:

> thanks, this looks good in general, but it would be nice to make sure we are 
> not blowing the index memory and disk usage.
>
> could you grab some numbers for these two before/after this patch?


Project: LLVM with clang and clang-tools-extra (~3700 records in 
compile_commands.json)

**Before:**
Index size on disk: 96461310 bytes
BackgroundIndex: serving version 39 (596622326 bytes)

**After:**
Index size of disk: 97425346 bytes.
BackgroundIndex: serving version 39 (601094341 bytes)

**Diff:**
Size on disk: +1%
Memory:   +0.7%

> this isn't really necessary as we are not changing the serialization format. 
> you would need to delete the existing clangd caches from your machine instead.

Removed version bump.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84513



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

[PATCH] D84513: [clangd] Collect references for externally visible main-file symbols

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX updated this revision to Diff 280713.
ArcsinX added a comment.

Remove version bump


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84513

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -624,11 +624,13 @@
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "NS").ID, _;
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "MACRO").ID,
   HaveRanges(Main.ranges("macro");
-  // Symbols *only* in the main file (a, b, c, FUNC) had no refs collected.
+  // Symbols *only* in the main file:
+  // - (a, b) externally visible and should have refs.
+  // - (c, FUNC) externally invisible and had no refs collected.
   auto MainSymbols =
   TestTU::withHeaderCode(SymbolsOnlyInMainCode.code()).headerSymbols();
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "a").ID, _;
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "b").ID, _;
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "a").ID, _)));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "b").ID, _)));
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "c").ID, _;
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "FUNC").ID, 
_;
 }
@@ -816,11 +818,15 @@
 $Foo[[Foo]] fo;
   }
   )");
-  // The main file is normal .cpp file, we shouldn't collect any refs of 
symbols
-  // which are not declared in the preamble.
+  // The main file is normal .cpp file, we should collect the refs
+  // for externally visible symbols.
   TestFileName = testPath("foo.cpp");
   runSymbolCollector("", Header.code());
-  EXPECT_THAT(Refs, UnorderedElementsAre());
+  EXPECT_THAT(Refs,
+  UnorderedElementsAre(Pair(findSymbol(Symbols, "Foo").ID,
+HaveRanges(Header.ranges("Foo"))),
+   Pair(findSymbol(Symbols, "Func").ID,
+HaveRanges(Header.ranges("Func");
 
   // Run the .h file as main file, we should collect the refs.
   TestFileName = testPath("foo.h");
Index: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
@@ -171,7 +171,7 @@
   #endif
   )cpp";
   FS.Files[testPath("root/A.cc")] =
-  "#include \"A.h\"\nvoid g() { (void)common; }";
+  "#include \"A.h\"\nstatic void g() { (void)common; }";
   FS.Files[testPath("root/B.cc")] =
   R"cpp(
   #define A 0
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -314,7 +314,8 @@
   // file locations for references (as it aligns the behavior of clangd's
   // AST-based xref).
   // FIXME: we should try to use the file locations for other fields.
-  if (CollectRef && !IsMainFileOnly && !isa(ND) &&
+  if (CollectRef && (!IsMainFileOnly || ND->isExternallyVisible()) &&
+  !isa(ND) &&
   (Opts.RefsInHeaders ||
SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
 DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -624,11 +624,13 @@
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(Symbols, "NS").ID, _;
   EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "MACRO").ID,
   HaveRanges(Main.ranges("macro");
-  // Symbols *only* in the main file (a, b, c, FUNC) had no refs collected.
+  // Symbols *only* in the main file:
+  // - (a, b) externally visible and should have refs.
+  // - (c, FUNC) externally invisible and had no refs collected.
   auto MainSymbols =
   TestTU::withHeaderCode(SymbolsOnlyInMainCode.code()).headerSymbols();
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "a").ID, _;
-  EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "b").ID, _;
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "a").ID, _)));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(MainSymbols, "b").ID, _)));
   EXPECT_THAT(Refs, Not(Con

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

[Vaibhav Garg via Phabricator via cfe-commits]

gargvaibhav64 added a comment.

Hi everyone, are there any more changes required to this review?


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

https://reviews.llvm.org/D83174



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