[PATCH] D59605: [clangd] Introduce background-indexer

2019-04-07 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

I would propose to change the name of the tool.
The "background" part only makes sense in the context of running inside clangd, 
the index stored on disk is not "background" in any manner.
I'd go with something like `build-clangd-index` (the best option is probably 
`clangd-indexer`, but it's is already taken and we don't want to confuse the 
two tools).

Also not sure whether the `BackgroundIndex` is the right abstraction to use 
here. Conceptually, we want everything from the background index, except the 
"background" part and the fact that it listens to updates for compile commands 
(more stuff might get inconsistent in the future, e.g if we start watching for 
changes to headers, we would probably want to disable this functionality here). 
I.e. we went with `BackgroundIndex` for code reuse, not because it's the right 
abstraction for the job. I do think we could come up with something sensible.

That said, the right abstraction is not there now and it may take considerable 
refactoring to come up with one. And the tool seems to be useful regardless of 
the underlying implementation, not exactly sure what's the balance we should 
strike here (time spent vs useful results).

Kadir, what are your thoughts on that matter?




Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:29
+
+static llvm::cl::opt CompileCommandsDir(
+llvm::cl::Positional,

That's not the accurate name or description, right?
We would actually walk up the directory tree as normal and discover any 
compilation database up the tree, right?

Maybe rename and document accordingly?



Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:40
+
+  llvm::errs().SetBuffered();
+  clang::clangd::StreamLogger Logger(llvm::errs(), 
clang::clangd::Logger::Info);

Why? Maybe add a comment?



Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:47
+  clang::clangd::OverlayCDB CDB(&DirectoryBasedCDB);
+  // FIXME: Maybe add a way to not lower thread priority?
+  clang::clangd::BackgroundIndex BackgroundIdx(

I think that's actually a hard requirement.
We definitely want to run with normal priority when running as a separate tool.



Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:56
+  // non-interactive tools like this one.
+  24 * 60 * 60 * 1000);
+

Maybe use `std::numeric_limits::max()` here?
Would mean we'll never build the dex index, exactly what we want here.



Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:65
+  llvm::sys::path::append(DummyFile, "dummy.cpp");
+  CDB.getCompileCommand(DummyFile);
+

We seem to be fighting with the interface we defined here.
If we need to support an operation of re-building the index for the underlying 
CDB, let's add it explicitly.



Comment at: clang-tools-extra/clangd/background-indexer/BackgroundIndexer.cpp:68
+  // Wait untill indexing finishes.
+  BackgroundIdx.blockUntilIdleForTest(llvm::None);
+  return 0;

We can't leave the function called like this, since we now seem to have a 
legitimate usage outside the test code.

Maybe rename it accordingly?



Comment at: clang-tools-extra/clangd/index/Background.h:149
+  // For logging
+  std::atomic EnqueuedTUs{0};
+  std::atomic IndexedTUs{0};

This change is not related to the rest of the patch.
Even adding some stats makes sense, could you move into a separate change to 
keep this change minimal?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59605



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-07 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added inline comments.



Comment at: clangd/ClangdUnit.cpp:405
 
-  std::vector Diags = ASTDiags.take();
+  auto ShouldSurfaceDiag = [](const Diag &D) {
+if (D.Severity == DiagnosticsEngine::Level::Error ||

The name suggests we filter **all** diagnostics based on this helper.
Maybe add rename to something more specific?

E.g. `IsErrorOrFatal` or `IsImporantHeaderDiagnostic`...



Comment at: clangd/ClangdUnit.cpp:408
+D.Severity == DiagnosticsEngine::Level::Fatal)
+  return true;
+return false;

NIT: simplify to 
```
return D.Severity == DiagnosticsEngine::Level::Error
|| D.Severity == DiagnosticsEngine::Level::Fatal
```



Comment at: clangd/ClangdUnit.cpp:414
+  llvm::DenseMap NumberOfDiagsAtLine;
+  auto TryAddDiag = [&Diags, &NumberOfDiagsAtLine](const Diag &D) {
+if (++NumberOfDiagsAtLine[D.Range.start.line] > 1) {

NIT: rename to `AddDiagnosticFromInclude` or something similar, but more 
specific than the current name



Comment at: clangd/ClangdUnit.cpp:425
+  Diags.push_back(D);
+else if (ShouldSurfaceDiag(D))
+  TryAddDiag(D);

Why not merge the `ShouldSurfaceDiag` and `TryAddDiag` into a single function 
and handle all the complexities there?
This would simplify the client code, it would be as simple as 
```
else
 AddDiagnosticFromInclude(D)
```



Comment at: clangd/ClangdUnit.cpp:434
+  if (Preamble) {
+for (const Diag &D : Preamble->Diags) {
+  if (mentionsMainFile(D))

Can we do this in `StoreDiags::flushLastDiag`?
All code handling the diagnostics lives there and it has all the information 
necessary to map to the included line.



Comment at: clangd/ClangdUnit.cpp:445
+  // same line.
+  for (auto &D : Diags) {
+if (!mentionsMainFile(D)) {

This extra complexity does not seem to be worth it after all.
I'd suggest to remove this (at least from the first version of the file), even 
though I was the one who proposed it.

Still think it's valuable, but the patch is complicated enough as it is, 
keeping it simpler seems to be more important at this point.



Comment at: clangd/Diagnostics.cpp:396
+  for (llvm::StringRef Inc : IncludeStack)
+OS << "In file included from: " << Inc << ":\n";
+}

kadircet wrote:
> ilya-biryukov wrote:
> > NIT: could we reuse the function from clang that does the same thing?
> > 
> > The code here is pretty simple, though, so writing it here is fine. Just 
> > wanted to make sure we considered this option and found it's easier to redo 
> > this work ourselves.
> there is `TextDiagnostic` which prints the desired output expect the fact 
> that it also prints the first line saying the header was included in main 
> file. Therefore, I didn't make use of it since we decided that was not very 
> useful for our use case. And it requires inheriting from `DiagnosticRenderer` 
> which was requiring too much boiler plate code just to get this functionality 
> so instead I've chosen doing it like this.
> 
> Can fallback to `TextDiagnostic` if you believe showing `Included in 
> mainfile.cc:x:y:` at the beginning of the diagnostic won't be annoying.
LG, it's not too hard to reconstruct the same output.
Note that D60267 starts outputting notes in a structured way, using the 
`RelatedInformation` struct from the LSP.

We should eventually add the include stack into related information too. With 
that in mind, we should probably add the include stack as a new field to the 
struct we use for diagnostics.




Comment at: clangd/Diagnostics.cpp:371
 FillDiagBase(*LastDiag);
+auto IncludeLocation = Info.getSourceManager()
+   .getPresumedLoc(Info.getLocation(),

That's a lot of code, could we extract this into a separate function?



Comment at: clangd/Diagnostics.cpp:372
+auto IncludeLocation = Info.getSourceManager()
+   .getPresumedLoc(Info.getLocation(),
+   /*UseLineDirectives=*/false)

Why use `getPresumedLoc`? Making clangd sensitive to pp directives that rename 
file or change line numbers does not make any sense.

Could you also add tests for that?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D60379: Make precompiled headers reproducible

2019-04-07 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

Needs a test.




Comment at: lib/Serialization/ASTWriter.cpp:4283
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {

Would it be better to just change the actual type of `SemaRef.OpenCLTypeExtMap`?
https://github.com/llvm-mirror/clang/blob/18917301298ad6df9e989983ed1e22cb0f9dff29/include/clang/Sema/Sema.h#L8710-L8712


Repository:
  rC Clang

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

https://reviews.llvm.org/D60379



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

r357879 - IAS is now enabled for all OS on MIPS64

2019-04-07 Thread Brad Smith via cfe-commits 
Author: brad
Date: Sun Apr  7 17:03:01 2019
New Revision: 357879

URL: http://llvm.org/viewvc/llvm-project?rev=357879&view=rev
Log:
IAS is now enabled for all OS on MIPS64

Modified:
cfe/trunk/test/Driver/freebsd.c
cfe/trunk/test/Driver/openbsd.c

Modified: cfe/trunk/test/Driver/freebsd.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/freebsd.c?rev=357879&r1=357878&r2=357879&view=diff
==
--- cfe/trunk/test/Driver/freebsd.c (original)
+++ cfe/trunk/test/Driver/freebsd.c Sun Apr  7 17:03:01 2019
@@ -184,11 +184,7 @@
 // RUN:   | FileCheck -check-prefix=CHECK-MIPS64-CPU %s
 // CHECK-MIPS64-CPU: "-target-cpu" "mips3"
 
-// Check that the integrated assembler is enabled for MIPS64/SPARC64
-// RUN: %clang -target mips64-unknown-freebsd -### -c %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-IAS %s
-// RUN: %clang -target mips64el-unknown-freebsd -### -c %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-IAS %s
+// Check that the integrated assembler is enabled for SPARC64
 // RUN: %clang -target sparc64-unknown-freebsd -### -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-IAS %s
 // CHECK-IAS-NOT: "-no-integrated-as"

Modified: cfe/trunk/test/Driver/openbsd.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/openbsd.c?rev=357879&r1=357878&r2=357879&view=diff
==
--- cfe/trunk/test/Driver/openbsd.c (original)
+++ cfe/trunk/test/Driver/openbsd.c Sun Apr  7 17:03:01 2019
@@ -74,11 +74,7 @@
 // CHECK-MIPS64EL: as{{.*}}" "-mabi" "64" "-EL"
 // CHECK-MIPS64EL-PIC: as{{.*}}" "-mabi" "64" "-EL" "-KPIC"
 
-// Check that the integrated assembler is enabled for MIPS64/SPARC
-// RUN: %clang -target mips64-unknown-openbsd -### -c %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-IAS %s
-// RUN: %clang -target mips64el-unknown-openbsd -### -c %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-IAS %s
+// Check that the integrated assembler is enabled for SPARC
 // RUN: %clang -target sparc-unknown-openbsd -### -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-IAS %s
 // RUN: %clang -target sparc64-unknown-openbsd -### -c %s 2>&1 \


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

r357878 - Enable IAS for FreeBSD SPARC64.

2019-04-07 Thread Brad Smith via cfe-commits 
Author: brad
Date: Sun Apr  7 16:12:31 2019
New Revision: 357878

URL: http://llvm.org/viewvc/llvm-project?rev=357878&view=rev
Log:
Enable IAS for FreeBSD SPARC64.

Modified:
cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
cfe/trunk/test/Driver/freebsd.c

Modified: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Gnu.cpp?rev=357878&r1=357877&r2=357878&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp Sun Apr  7 16:12:31 2019
@@ -2547,7 +2547,8 @@ bool Generic_GCC::IsIntegratedAssemblerD
   case llvm::Triple::sparc:
   case llvm::Triple::sparcel:
   case llvm::Triple::sparcv9:
-if (getTriple().isOSSolaris() || getTriple().isOSOpenBSD())
+if (getTriple().isOSFreeBSD() || getTriple().isOSOpenBSD() ||
+getTriple().isOSSolaris())
   return true;
 return false;
   default:

Modified: cfe/trunk/test/Driver/freebsd.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/freebsd.c?rev=357878&r1=357877&r2=357878&view=diff
==
--- cfe/trunk/test/Driver/freebsd.c (original)
+++ cfe/trunk/test/Driver/freebsd.c Sun Apr  7 16:12:31 2019
@@ -184,9 +184,11 @@
 // RUN:   | FileCheck -check-prefix=CHECK-MIPS64-CPU %s
 // CHECK-MIPS64-CPU: "-target-cpu" "mips3"
 
-// Check that the integrated assembler is enabled for MIPS64
+// Check that the integrated assembler is enabled for MIPS64/SPARC64
 // RUN: %clang -target mips64-unknown-freebsd -### -c %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-MIPS64-AS %s
+// RUN:   | FileCheck -check-prefix=CHECK-IAS %s
 // RUN: %clang -target mips64el-unknown-freebsd -### -c %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-MIPS64-AS %s
-// CHECK-MIPS64-AS-NOT: "-no-integrated-as"
+// RUN:   | FileCheck -check-prefix=CHECK-IAS %s
+// RUN: %clang -target sparc64-unknown-freebsd -### -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-IAS %s
+// CHECK-IAS-NOT: "-no-integrated-as"


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

[PATCH] D60335: Use -fomit-frame-pointer when optimizing PowerPC code

2019-04-07 Thread Brad Smith via Phabricator via cfe-commits 
brad added a comment.

George, look at adding some appropriate entries to 
test/Driver/frame-pointer-elim.c


Repository:
  rC Clang

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

https://reviews.llvm.org/D60335



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

[PATCH] D60380: Also document -arch as -arch is mac specific

2019-04-07 Thread Sylvestre Ledru via Phabricator via cfe-commits 
sylvestre.ledru created this revision.
sylvestre.ledru added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

https://reviews.llvm.org/D60380

Files:
  docs/CommandGuide/clang.rst


Index: docs/CommandGuide/clang.rst
===
--- docs/CommandGuide/clang.rst
+++ docs/CommandGuide/clang.rst
@@ -312,7 +312,11 @@
 
 .. option:: -arch 
 
-  Specify the architecture to build for.
+  Specify the architecture to build for (Mac OS X specific)
+
+.. option:: -target 
+
+  Specify the architecture to build for (Linux and others)
 
 .. option:: -mmacosx-version-min=
 


Index: docs/CommandGuide/clang.rst
===
--- docs/CommandGuide/clang.rst
+++ docs/CommandGuide/clang.rst
@@ -312,7 +312,11 @@
 
 .. option:: -arch 
 
-  Specify the architecture to build for.
+  Specify the architecture to build for (Mac OS X specific)
+
+.. option:: -target 
+
+  Specify the architecture to build for (Linux and others)
 
 .. option:: -mmacosx-version-min=
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60379: Make precompiled headers reproducible

2019-04-07 Thread Sylvestre Ledru via Phabricator via cfe-commits 
sylvestre.ledru created this revision.
sylvestre.ledru added reviewers: yaxunl, Anastasia.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch have been applied to Debian packages for a few months without
any regression.

Patch by Rebecca N. Palmer


Repository:
  rC Clang

https://reviews.llvm.org/D60379

Files:
  lib/Serialization/ASTWriter.cpp


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -4279,9 +4279,13 @@
 return;
 
   RecordData Record;
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {
-Record.push_back(
-static_cast(getTypeID(I.first->getCanonicalTypeInternal(;
+
sortedOpenCLTypeExtMap[getTypeID(I.first->getCanonicalTypeInternal())]=I.second;
+  }
+  for (const auto &I : sortedOpenCLTypeExtMap) {
+Record.push_back(static_cast(I.first));
 Record.push_back(I.second.size());
 for (auto Ext : I.second)
   AddString(Ext, Record);
@@ -4294,8 +4298,12 @@
 return;
 
   RecordData Record;
+  std::map> sortedOpenCLDeclExtMap;
   for (const auto &I : SemaRef.OpenCLDeclExtMap) {
-Record.push_back(getDeclID(I.first));
+sortedOpenCLDeclExtMap[getDeclID(I.first)]=I.second;
+  }
+  for (const auto &I : sortedOpenCLDeclExtMap) {
+Record.push_back(I.first);
 Record.push_back(static_cast(I.second.size()));
 for (auto Ext : I.second)
   AddString(Ext, Record);


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -4279,9 +4279,13 @@
 return;
 
   RecordData Record;
+  // Sort to allow reproducible .pch files - https://bugs.debian.org/877359
+  std::map> sortedOpenCLTypeExtMap;
   for (const auto &I : SemaRef.OpenCLTypeExtMap) {
-Record.push_back(
-static_cast(getTypeID(I.first->getCanonicalTypeInternal(;
+sortedOpenCLTypeExtMap[getTypeID(I.first->getCanonicalTypeInternal())]=I.second;
+  }
+  for (const auto &I : sortedOpenCLTypeExtMap) {
+Record.push_back(static_cast(I.first));
 Record.push_back(I.second.size());
 for (auto Ext : I.second)
   AddString(Ext, Record);
@@ -4294,8 +4298,12 @@
 return;
 
   RecordData Record;
+  std::map> sortedOpenCLDeclExtMap;
   for (const auto &I : SemaRef.OpenCLDeclExtMap) {
-Record.push_back(getDeclID(I.first));
+sortedOpenCLDeclExtMap[getDeclID(I.first)]=I.second;
+  }
+  for (const auto &I : sortedOpenCLDeclExtMap) {
+Record.push_back(I.first);
 Record.push_back(static_cast(I.second.size()));
 for (auto Ext : I.second)
   AddString(Ext, Record);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

2019-04-07 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added a comment.

In D59673#1452269 , @dblaikie wrote:

> Ah, fair. You could actually test the dwo_name is accurate in the .dwo file 
> (I added the dwo_name to the .dwo file so that multi-level dwp error messages 
> could be more informative)


Ok, I'll just check the dwo_name for both files then.

>>> (& I guess there's an LLVM patch to add the rest of this functionality?)
>> 
>> What do we have to do there? Should we add the option to `llc` as well?
> 
> Yep - pretty much anything in MCOptions should be surfaced through llc for 
> llvm-level testing.

I was about to add this when I discovered that there is such an option already. 
The command line `clang -cc1 -split-dwarf-file a -fsplit-dwarf-dwo-name-attr b` 
corresponds to `llc -split-dwarf-output a -split-dwarf-file b`. That seems a 
bit unfortunate, especially the different meanings of `-split-dwarf-file`. 
Should/can we strive to emulate the behavior of llc in Clang instead? That 
would perhaps require changes to the behavior of `-split-dwarf-file`. Here's 
how the options behave with llc:

| Option  | ∅| `-split-dwarf-file a`
|
| ∅   | No split | Split DI in same object, set 
`DW_AT_GNU_dwo_name = a`, no extra file |
| `-split-dwarf-output b` | No split | Split DI, set `DW_AT_GNU_dwo_name = a`, 
separate file `b`|
|

Currently, Clang's `-enable-split-dwarf[=split] -split-dwarf-file a` does the 
same as `-split-dwarf-output a -split-dwarf-file a` in llc, and 
`-enable-split-dwarf=single -split-dwarf-file a` does the same as 
`-split-dwarf-file a` in llc.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread Owen Pan via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL357877: [clang-format] Fix bug 
https://bugs.llvm.org/show_bug.cgi?id=41413 (authored by owenpan, committed by 
).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60374?vs=194072&id=194073#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60374

Files:
  cfe/trunk/lib/Format/ContinuationIndenter.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp


Index: cfe/trunk/lib/Format/ContinuationIndenter.cpp
===
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp
@@ -945,18 +945,24 @@
   return State.Stack[State.Stack.size() - 2].LastSpace;
 return State.FirstIndent;
   }
-  // Indent a closing parenthesis at the previous level if followed by a semi 
or
-  // opening brace. This allows indentations such as:
+  // Indent a closing parenthesis at the previous level if followed by a semi,
+  // const, or opening brace. This allows indentations such as:
   // foo(
   //   a,
   // );
+  // int Foo::getter(
+  // //
+  // ) const {
+  //   return foo;
+  // }
   // function foo(
   //   a,
   // ) {
   //   code(); //
   // }
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
-  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::l_brace)))
+  (!Current.Next ||
+   Current.Next->isOneOf(tok::semi, tok::kw_const, tok::l_brace)))
 return State.Stack[State.Stack.size() - 2].LastSpace;
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
 return State.Stack[State.Stack.size() - 2].LastSpace;
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -12822,6 +12822,24 @@
 format("int i = longFunction(arg);", SixIndent));
 }
 
+TEST_F(FormatTest, WrappedClosingParenthesisIndent) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat(
+  "int Foo::getter(\n"
+  "//\n"
+  ") const {\n"
+  "  return foo;\n"
+  "}",
+  Style);
+  verifyFormat(
+  "void Foo::setter(\n"
+  "//\n"
+  ") {\n"
+  "  foo = 1;\n"
+  "}",
+  Style);
+}
+
 TEST_F(FormatTest, SpacesInAngles) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInAngles = true;


Index: cfe/trunk/lib/Format/ContinuationIndenter.cpp
===
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp
@@ -945,18 +945,24 @@
   return State.Stack[State.Stack.size() - 2].LastSpace;
 return State.FirstIndent;
   }
-  // Indent a closing parenthesis at the previous level if followed by a semi or
-  // opening brace. This allows indentations such as:
+  // Indent a closing parenthesis at the previous level if followed by a semi,
+  // const, or opening brace. This allows indentations such as:
   // foo(
   //   a,
   // );
+  // int Foo::getter(
+  // //
+  // ) const {
+  //   return foo;
+  // }
   // function foo(
   //   a,
   // ) {
   //   code(); //
   // }
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
-  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::l_brace)))
+  (!Current.Next ||
+   Current.Next->isOneOf(tok::semi, tok::kw_const, tok::l_brace)))
 return State.Stack[State.Stack.size() - 2].LastSpace;
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
 return State.Stack[State.Stack.size() - 2].LastSpace;
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -12822,6 +12822,24 @@
 format("int i = longFunction(arg);", SixIndent));
 }
 
+TEST_F(FormatTest, WrappedClosingParenthesisIndent) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat(
+  "int Foo::getter(\n"
+  "//\n"
+  ") const {\n"
+  "  return foo;\n"
+  "}",
+  Style);
+  verifyFormat(
+  "void Foo::setter(\n"
+  "//\n"
+  ") {\n"
+  "  foo = 1;\n"
+  "}",
+  Style);
+}
+
 TEST_F(FormatTest, SpacesInAngles) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInAngles = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r357877 - [clang-format] Fix bug https://bugs.llvm.org/show_bug.cgi?id=41413

2019-04-07 Thread Owen Pan via cfe-commits 
Author: owenpan
Date: Sun Apr  7 14:05:52 2019
New Revision: 357877

URL: http://llvm.org/viewvc/llvm-project?rev=357877&view=rev
Log:
[clang-format] Fix bug https://bugs.llvm.org/show_bug.cgi?id=41413

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

Modified:
cfe/trunk/lib/Format/ContinuationIndenter.cpp
cfe/trunk/unittests/Format/FormatTest.cpp

Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=357877&r1=357876&r2=357877&view=diff
==
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Sun Apr  7 14:05:52 2019
@@ -945,18 +945,24 @@ unsigned ContinuationIndenter::getNewLin
   return State.Stack[State.Stack.size() - 2].LastSpace;
 return State.FirstIndent;
   }
-  // Indent a closing parenthesis at the previous level if followed by a semi 
or
-  // opening brace. This allows indentations such as:
+  // Indent a closing parenthesis at the previous level if followed by a semi,
+  // const, or opening brace. This allows indentations such as:
   // foo(
   //   a,
   // );
+  // int Foo::getter(
+  // //
+  // ) const {
+  //   return foo;
+  // }
   // function foo(
   //   a,
   // ) {
   //   code(); //
   // }
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
-  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::l_brace)))
+  (!Current.Next ||
+   Current.Next->isOneOf(tok::semi, tok::kw_const, tok::l_brace)))
 return State.Stack[State.Stack.size() - 2].LastSpace;
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
 return State.Stack[State.Stack.size() - 2].LastSpace;

Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=357877&r1=357876&r2=357877&view=diff
==
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Sun Apr  7 14:05:52 2019
@@ -12822,6 +12822,24 @@ TEST_F(FormatTest, ConfigurableContinuat
 format("int i = longFunction(arg);", SixIndent));
 }
 
+TEST_F(FormatTest, WrappedClosingParenthesisIndent) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat(
+  "int Foo::getter(\n"
+  "//\n"
+  ") const {\n"
+  "  return foo;\n"
+  "}",
+  Style);
+  verifyFormat(
+  "void Foo::setter(\n"
+  "//\n"
+  ") {\n"
+  "  foo = 1;\n"
+  "}",
+  Style);
+}
+
 TEST_F(FormatTest, SpacesInAngles) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInAngles = true;


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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread Owen Pan via Phabricator via cfe-commits 
owenpan requested review of this revision.
owenpan added a comment.

Updated the diff.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread Owen Pan via Phabricator via cfe-commits 
owenpan updated this revision to Diff 194072.
This revision is now accepted and ready to land.

Repository:
  rC Clang

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

https://reviews.llvm.org/D60374

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12822,6 +12822,24 @@
 format("int i = longFunction(arg);", SixIndent));
 }
 
+TEST_F(FormatTest, WrappedClosingParenthesisIndent) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat(
+  "int Foo::getter(\n"
+  "//\n"
+  ") const {\n"
+  "  return foo;\n"
+  "}",
+  Style);
+  verifyFormat(
+  "void Foo::setter(\n"
+  "//\n"
+  ") {\n"
+  "  foo = 1;\n"
+  "}",
+  Style);
+}
+
 TEST_F(FormatTest, SpacesInAngles) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInAngles = true;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -945,18 +945,24 @@
   return State.Stack[State.Stack.size() - 2].LastSpace;
 return State.FirstIndent;
   }
-  // Indent a closing parenthesis at the previous level if followed by a semi 
or
-  // opening brace. This allows indentations such as:
+  // Indent a closing parenthesis at the previous level if followed by a semi,
+  // const, or opening brace. This allows indentations such as:
   // foo(
   //   a,
   // );
+  // int Foo::getter(
+  // //
+  // ) const {
+  //   return foo;
+  // }
   // function foo(
   //   a,
   // ) {
   //   code(); //
   // }
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
-  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::l_brace)))
+  (!Current.Next ||
+   Current.Next->isOneOf(tok::semi, tok::kw_const, tok::l_brace)))
 return State.Stack[State.Stack.size() - 2].LastSpace;
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
 return State.Stack[State.Stack.size() - 2].LastSpace;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12822,6 +12822,24 @@
 format("int i = longFunction(arg);", SixIndent));
 }
 
+TEST_F(FormatTest, WrappedClosingParenthesisIndent) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat(
+  "int Foo::getter(\n"
+  "//\n"
+  ") const {\n"
+  "  return foo;\n"
+  "}",
+  Style);
+  verifyFormat(
+  "void Foo::setter(\n"
+  "//\n"
+  ") {\n"
+  "  foo = 1;\n"
+  "}",
+  Style);
+}
+
 TEST_F(FormatTest, SpacesInAngles) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInAngles = true;
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -945,18 +945,24 @@
   return State.Stack[State.Stack.size() - 2].LastSpace;
 return State.FirstIndent;
   }
-  // Indent a closing parenthesis at the previous level if followed by a semi or
-  // opening brace. This allows indentations such as:
+  // Indent a closing parenthesis at the previous level if followed by a semi,
+  // const, or opening brace. This allows indentations such as:
   // foo(
   //   a,
   // );
+  // int Foo::getter(
+  // //
+  // ) const {
+  //   return foo;
+  // }
   // function foo(
   //   a,
   // ) {
   //   code(); //
   // }
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
-  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::l_brace)))
+  (!Current.Next ||
+   Current.Next->isOneOf(tok::semi, tok::kw_const, tok::l_brace)))
 return State.Stack[State.Stack.size() - 2].LastSpace;
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
 return State.Stack[State.Stack.size() - 2].LastSpace;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay added a comment.

In D60374#1457705 , @owenpan wrote:

> In D60374#1457693 , @MyDeveloperDay 
> wrote:
>
> > LGTM , if you also think the test will help show the use case then please 
> > add it, otherwise this revision notes might be information enough
>
>
> Thanks! I just got the alternative patch ready. Should I discard it and just 
> commit this one?


Maybe just update this revision with the new diff.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread Owen Pan via Phabricator via cfe-commits 
owenpan added a comment.

In D60374#1457693 , @MyDeveloperDay 
wrote:

> LGTM , if you also think the test will help show the use case then please add 
> it, otherwise this revision notes might be information enough


Thanks! I just got the alternative patch ready. Should I discard it and just 
commit this one?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread Owen Pan via Phabricator via cfe-commits 
owenpan planned changes to this revision.
owenpan added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:968
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
-  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::l_brace)))
+  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::colon,
+  tok::kw_const, tok::l_brace)))

MyDeveloperDay wrote:
> Do you think the colon you have here in your example is really a 
> TT_CtorInitializerColon, I'm not sure if you need to be specific?
> 
> I wonder how this might interfere with the  
> Style.BreakConstructorInitializers,  I'm a little unclear of the before and 
> after you are trying to fix, could you add something to the description to 
> help me understand?
Good point with using TT_CtorInitializerColon instead!

Although I had tested it with different combinations of 
Style.BreakConstructorInitializers settings and didn't find any interference, 
it's probably not an inconsistency in the case of the colon based on your and 
@klimek's comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

LGTM , if you also think the test will help show the use case then please add 
it, otherwise this revision notes might be information enough


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread Owen Pan via Phabricator via cfe-commits 
owenpan added a comment.

In D60374#1457639 , @MyDeveloperDay 
wrote:

> maybe add the following as a test because I think it shows the inconsistency
>
>   void Foo::bar( // some comment
>   ) {}
>  
>   void Foo::bar( // some comment
>   ) const {}
>


I had the `setter` code in the new test case to show the inconsistency but then 
took it out; I don't want to add a (possible) duplicate even though I didn't 
find the test case for Lines 953-957 of `ContinuationIndenter.cpp`. I will put 
my `setter` test case back in.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread Owen Pan via Phabricator via cfe-commits 
owenpan added a comment.

In D60374#1457638 , @klimek wrote:

> The previous behavior looks intentional, and much more regular. I'd be 
> curious why you think the proposed behavior is more readable.


I see the bug in the inconsistency for the code below:

  int Foo::getter(
  //
  ) const {
return foo;
  }
  
  void Foo::setter(
  //
  ) {
foo = 1;
  }

The closing parenthesis is indented in `getter` but not `setter`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60281: [analyzer] Add docs for cplusplus.InnerPointer

2019-04-07 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

Woohoo!




Comment at: docs/analyzer/checkers.rst:225-226
+``std::string``s, by recognizing member functions that may re/deallocate the 
buffer
+before use. In the future, it would be great to add support for other STL and
+non-STL containers, and most notably, ``std::string_view``s.
+

Hmm. While this page is a documentation, I would still expect regular users to 
browse through it -- are we sure that we want to add future plans for a 
non-alpha checker? I'm not against it, just a question.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60281



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

Re: [PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

2019-04-07 Thread Angus Hewlett via cfe-commits 
Even if I just issue one file with /Yc and none with /Yu, it still fails.
Looks like it possibly generates two processes, one to generate the pch and
the other to compile and link. A temp file is generated but no final pch;
an error is emitted "Unable to read pch file foo.pch".

On Sun, 7 Apr 2019, 14:19 Alexandre Ganea via Phabricator, <
revi...@reviews.llvm.org> wrote:

> aganea added a comment.
>
> That is strange. As you can’t mix /Yc and /Yu on the command line, MSBuild
> should issue two different calls to clang-cl, one with /Yc and one /Yu /MP.
> Can you check with ProcMon that commands are issued in the right order?
>
>
> Repository:
>   rC Clang
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D52193/new/
>
> https://reviews.llvm.org/D52193
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay added a comment.

maybe add the following as a test because I think it shows the inconsistency

  void Foo::bar( // some comment
  ) {}
  
  void Foo::bar( // some comment
  ) const {}


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread Manuel Klimek via Phabricator via cfe-commits 
klimek added a comment.

The previous behavior looks intentional, and much more regular. I'd be curious 
why you think the proposed behavior is more readable.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

2019-04-07 Thread Paul Robinson via Phabricator via cfe-commits 
probinson added inline comments.



Comment at: test/CodeGenCXX/debug-info-composite-triviality.cpp:88
-
-// CHECK-DAG: !DICompositeType({{.*}}, name: "NonTrivialE",{{.*}}flags: 
{{.*}}DIFlagNonTrivial
-struct NonTrivialE : Trivial, NonTrivial {

Hui wrote:
> probinson wrote:
> > This one used to be flagged nontrivial, and now it's not flagged?
> The base struct 'Trivial' was removed  and this case became duplicated since 
> it is now similar with 'struct NonTrivialD'. Could be very easy to add it 
> back per request.
I think it's worth having a multiple-inheritance case.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59347



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

2019-04-07 Thread Alexandre Ganea via Phabricator via cfe-commits 
aganea added a comment.

That is strange. As you can’t mix /Yc and /Yu on the command line, MSBuild 
should issue two different calls to clang-cl, one with /Yc and one /Yu /MP. Can 
you check with ProcMon that commands are issued in the right order?


Repository:
  rC Clang

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

https://reviews.llvm.org/D52193



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

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

2019-04-07 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:968
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
-  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::l_brace)))
+  (!Current.Next || Current.Next->isOneOf(tok::semi, tok::colon,
+  tok::kw_const, tok::l_brace)))

Do you think the colon you have here in your example is really a 
TT_CtorInitializerColon, I'm not sure if you need to be specific?

I wonder how this might interfere with the  Style.BreakConstructorInitializers, 
 I'm a little unclear of the before and after you are trying to fix, could you 
add something to the description to help me understand?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60374



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

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

2019-04-07 Thread Reuben Thomas via Phabricator via cfe-commits 
reuk updated this revision to Diff 194057.
reuk added a comment.

Fixed formatting nit.


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

https://reviews.llvm.org/D60320

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9719,6 +9719,17 @@
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
+TEST_F(FormatTest, SpaceAfterLogicalNot) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpaceAfterLogicalNot = true;
+
+  verifyFormat("bool x = ! y", Spaces);
+  verifyFormat("if (! isFailure())", Spaces);
+  verifyFormat("if (! (a && b))", Spaces);
+  verifyFormat("\"Error!\"", Spaces);
+  verifyFormat("! ! x", Spaces);
+}
+
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
   FormatStyle Spaces = getLLVMStyle();
 
@@ -11511,6 +11522,12 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
+  Style.SpaceAfterLogicalNot = false;
+  CHECK_PARSE("SpaceAfterLogicalNot: true", SpaceAfterLogicalNot,
+  true);
+  CHECK_PARSE("SpaceAfterLogicalNot: false", SpaceAfterLogicalNot,
+  false);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2832,7 +2832,8 @@
 return true;
   }
   if (Left.is(TT_UnaryOperator))
-return Right.is(TT_BinaryOperator);
+return (Style.SpaceAfterLogicalNot && Left.is(tok::exclaim)) ||
+   Right.is(TT_BinaryOperator);
 
   // If the next token is a binary operator or a selector name, we have
   // incorrectly classified the parenthesis as a cast. FIXME: Detect correctly.
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -478,6 +478,7 @@
 IO.mapOptional("SortIncludes", Style.SortIncludes);
 IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);
 IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
+IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
 IO.mapOptional("SpaceAfterTemplateKeyword",
Style.SpaceAfterTemplateKeyword);
 IO.mapOptional("SpaceBeforeAssignmentOperators",
@@ -730,6 +731,7 @@
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
+  LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1628,6 +1628,13 @@
   /// \endcode
   bool SpaceAfterCStyleCast;
 
+  /// If ``true``, a space is inserted after the logical not operator (``!``).
+  /// \code
+  ///true:  false:
+  ///! someExpression();vs. !someExpression();
+  /// \endcode
+  bool SpaceAfterLogicalNot;
+
   /// If \c true, a space will be inserted after the 'template' keyword.
   /// \code
   ///true:  false:
@@ -1911,6 +1918,7 @@
PointerAlignment == R.PointerAlignment &&
RawStringFormats == R.RawStringFormats &&
SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
+   SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
SpaceBeforeCpp11BracedList == R.SpaceBeforeCpp11BracedList &&
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -1952,6 +1952,13 @@
  true:  false:
  (int) i;   vs. (int)i;
 
+**SpaceAfterLogicalNot** (``bool``)
+  If ``true``, a space is inserted after the logical not operator (``!``).
+  .. code-block:: c++
+
+ true:  false:
+ ! someExpression();vs. !someExpression();
+
 **SpaceAfterTemplateKeyword** (``bool``)
   If ``true``, a space will be inser

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

2019-04-07 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

LGTM (minor nit)




Comment at: clang/lib/Format/TokenAnnotator.cpp:2834
   }
-  if (Left.is(TT_UnaryOperator))
-return Right.is(TT_BinaryOperator);
+  if (Left.is(TT_UnaryOperator)) {
+return (Style.SpaceAfterLogicalNot && Left.is(tok::exclaim)) ||

nit: you don't need to {} 


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

https://reviews.llvm.org/D60320



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

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

2019-04-07 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay added a comment.

In D60320#1457568 , @reuk wrote:

> I updated `ClangFormatStyleOptions.rst` by hand, is there a way to do that 
> automatically instead (for future patches)?


I do it by hand, I know there is a python script out there, go with this for 
now, perhaps I'll try and find out how to do it automatically and then submit 
any differences if there are any (I'm sure there are some)


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

https://reviews.llvm.org/D60320



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

[PATCH] D59870: [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

2019-04-07 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:33
+
+  Upper limit for the magnitue bits of the loop variable. If it's set the check
+  filters out those catches in which the loop variable's type has more 
magnitude

ztamas wrote:
> JonasToth wrote:
> > typo `magnitude`
> > 
> > The commit you reference to the project fixing the "bugs" stated, that the 
> > version without conversions is more efficient (by one instruction :)). I 
> > think this might be worth a note, that even though the
> > code is not a bug in itself it might still be preferable to remove those 
> > code locations in the long run.
> Typo is fixed
> 
> The referenced commit message mentions 32 bit system, where changing integer 
> types was a bit faster. I'm not sure how general this is. I'm not familiar 
> with the low level implementation of integer comparison, so I would not 
> mention something, that I don't actually understand how it works.
I dont understand it either in detail, it sounded logical that you need to 
convert the different integer sizes first somehow, but I am fine with not 
mentioning it :)


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

https://reviews.llvm.org/D59870



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

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

2019-04-07 Thread Reuben Thomas via Phabricator via cfe-commits 
reuk added a comment.

I updated `ClangFormatStyleOptions.rst` by hand, is there a way to do that 
automatically instead (for future patches)?


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

https://reviews.llvm.org/D60320



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

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

2019-04-07 Thread Reuben Thomas via Phabricator via cfe-commits 
reuk updated this revision to Diff 194055.
reuk added a comment.

Updated with fixes. Thanks for pointing out that the options are ordered 
alphabetically, I hadn't noticed that!


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

https://reviews.llvm.org/D60320

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -9719,6 +9719,17 @@
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
+TEST_F(FormatTest, SpaceAfterLogicalNot) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpaceAfterLogicalNot = true;
+
+  verifyFormat("bool x = ! y", Spaces);
+  verifyFormat("if (! isFailure())", Spaces);
+  verifyFormat("if (! (a && b))", Spaces);
+  verifyFormat("\"Error!\"", Spaces);
+  verifyFormat("! ! x", Spaces);
+}
+
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
   FormatStyle Spaces = getLLVMStyle();
 
@@ -11511,6 +11522,12 @@
   CHECK_PARSE("SpaceAfterControlStatementKeyword: true", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
 
+  Style.SpaceAfterLogicalNot = false;
+  CHECK_PARSE("SpaceAfterLogicalNot: true", SpaceAfterLogicalNot,
+  true);
+  CHECK_PARSE("SpaceAfterLogicalNot: false", SpaceAfterLogicalNot,
+  false);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2831,8 +2831,10 @@
   return false;
 return true;
   }
-  if (Left.is(TT_UnaryOperator))
-return Right.is(TT_BinaryOperator);
+  if (Left.is(TT_UnaryOperator)) {
+return (Style.SpaceAfterLogicalNot && Left.is(tok::exclaim)) ||
+   Right.is(TT_BinaryOperator);
+  }
 
   // If the next token is a binary operator or a selector name, we have
   // incorrectly classified the parenthesis as a cast. FIXME: Detect correctly.
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -478,6 +478,7 @@
 IO.mapOptional("SortIncludes", Style.SortIncludes);
 IO.mapOptional("SortUsingDeclarations", Style.SortUsingDeclarations);
 IO.mapOptional("SpaceAfterCStyleCast", Style.SpaceAfterCStyleCast);
+IO.mapOptional("SpaceAfterLogicalNot", Style.SpaceAfterLogicalNot);
 IO.mapOptional("SpaceAfterTemplateKeyword",
Style.SpaceAfterTemplateKeyword);
 IO.mapOptional("SpaceBeforeAssignmentOperators",
@@ -730,6 +731,7 @@
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
+  LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1628,6 +1628,13 @@
   /// \endcode
   bool SpaceAfterCStyleCast;
 
+  /// If ``true``, a space is inserted after the logical not operator (``!``).
+  /// \code
+  ///true:  false:
+  ///! someExpression();vs. !someExpression();
+  /// \endcode
+  bool SpaceAfterLogicalNot;
+
   /// If \c true, a space will be inserted after the 'template' keyword.
   /// \code
   ///true:  false:
@@ -1911,6 +1918,7 @@
PointerAlignment == R.PointerAlignment &&
RawStringFormats == R.RawStringFormats &&
SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
+   SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
SpaceBeforeCpp11BracedList == R.SpaceBeforeCpp11BracedList &&
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -1952,6 +1952,13 @@
  true:  false:
  (int) i;   vs. (int)i;
 
+**SpaceAfterLogicalNot** (``bool``)
+  If ``true``, a space is inserted after the logical not operator (``!``).
+  .. code-block:: c++
+
+ true:

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

2019-04-07 Thread Angus Hewlett via Phabricator via cfe-commits 
angushewlett added a comment.

Having applied this patch to the clang latest (which took a bit of doing, and 
it's possible I messed up along the way) - all working in general, and a 
roughly 6x speed-up on my i7-7820X. However, PCH support does not seem to work 
correctly.

Specifically, it looks like when a file is set to build with /Yc (Create PCH), 
this is not executed serially with respect to the other jobs. Therefore it 
attempts to compile targets with /Yu (Use PCH) before the PCH has been 
generated, and fails.

When one or more files are passed with /Yc, PCH generation needs to execute to 
completion before the other jobs are started.

If I compile without using PCHs, it all works correctly (but clearly there's 
some performance hit).

Can anyone confirm if they've got /Yc & /Yu working correctly on their system?

Thanks,

  Angus.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52193



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

[PATCH] D54978: Move the SMT API to LLVM

2019-04-07 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: cfe/trunk/CMakeLists.txt:453
+if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or 
Z3")
 endif()

This message needs to be updated :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54978



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