[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

In D84362#2365153 , @rjmccall wrote:

> I may not have been clear. I'm not saying SourceLocation is a meaningful 
> concept in the driver.

Likewise, I should be more careful with respect to how I refer to 
`SourceLocation` (a class in Clang) and "source location" (location within a 
source file).

I see your point. Basically, you are suggesting that we improve the diagnostics 
for the driver first. And for that we'd need something like 
`CommandLineLocation` (a new class: location within the command invocation) 
that would be related to `SourceLocation` via a common base class, e.g. 
`AbstractLocation` (also a new class). That would indeed be a very nice thing 
to have.

However, this is not what we've been proposing. Our ultimate goal is a new 
Flang driver:

- implemented in terms of libclangDriver
- independent of Clang.

We shouldn't need to implement this extra functionality to achieve our goal. 
Personally I like the idea, but we need to stay focused on activities that are 
strictly needed for a new Flang driver. Not making things worse for Clang is an 
equally important goal. Improving things in Clang is something we are keen on, 
but are unlikely to have the bandwidth for.

It'll hard to create these thin layers above the diagnostic classes that do not 
depend on `SourceLocation` and `SourceManager` (and other classes that 
depend/require these). This is regardless of whether driver diagnostics are 
capable of tracking location (via e.g. `CommandLineLocation`) or not. The 
integration of diagnostics classes and "source location" related classes 
(specifically `SourceLocation` and `SourceManager`) is quite deep and this 
patch deepens that. IMO this patch makes things even harder for us. I'm 
starting to think that all this could be vastly simplified if libclangDriver 
had it's own diagnostics classes- a very simplified version of what's currently 
available in libclangBasic (again, because the driver doesn't need much). The 
improvements that you propose could be added at later time. Would this be 
acceptable? I think that the thin abstraction layer that we're discussing here 
would naturally emerge with time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I may not have been clear.  I'm not saying SourceLocation is a meaningful 
concept in the driver.  I'm saying that if you generalize the concept of 
"source location" to "location in the input", there is a clear analogue in the 
driver (namely, a position in the argument list), and the only reason this 
isn't passed down to the driver and used in diagnostics is that we don't have 
the ability to express that today to the diagnostic engine.  So instead of 
trying to extract out a part of the diagnostics engine that will work without 
any concept of source locations, you should be trying to parameterize the 
diagnostics engine so that it can work with an arbitrary external concept of 
source locations, and then the driver can use a different kind of source 
location than the main compiler and everything is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Hi @rjmccall , thank you for your quick reply!

> It sounds like what you want is a diagnostic library that's almost completely 
> abstracted over the kinds of entities that can be stored in a diagnostic, 
> including the definition of a source location.

No :) We suggested (and that's based on the feedback on cfe-dev) a more basic 
approach. We proposed an abstraction layer that does not require any location 
information, because that's irrelevant in the driver (more on this below). We 
did, intially, suggest re-using and re-factoring `SourceLocation` (and, more 
specifically, `SourceManager`). That direction was explicitely discouraged on 
cfe-dev [3].

> Source locations, supplemental source ranges, and fix-its are all still 
> meaningful concepts in the driver

When we studied the source code of libclangDriver that was not the case. The 
diagnostics in the driver [1] are only used to a very limited extent (compared 
to the frontend) and I've only seen them issued via [2]:

  inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
return Report(SourceLocation(), DiagID);
  }

i.e. `SourceLocation` is not really used at all. Are we missing something here? 
Is there any place in the libclangDriver (or any tool that's implemented in 
terms of libclangdDriver) that actually requires `SourceLocation`?

We would like to keep the scope of refactoring libclangDriver (and its 
dependencies) to the required minimum. The solution that you suggested would 
mean refactoring an API that's not required by libclangDriver (please correct 
me if I'm missing something here). I'm concerned that any attempts to modify 
`SourceLocation` will be challenged on cfe-dev. It was already suggested that 
we shouldn't need it for what we want to achieve. And indeed, in our simplified 
prototype downstream we were able to avoid using it without affecting the 
driver diagnostics.

Another difficulty with your suggestion is testing. If we were to create some 
abstraction layer above `SourceLocation` - how would we define or test it? 
Again, AFAIK diagnostics in libclangDriver never contain any information with 
respect to source location. So how do we decide what's actually required? This 
would be easy if there was a reference example :)

All in all, I think that this patch will require us to broaden the refactoring 
in a way that otherwise wouldn't be required. And according to the feedback so 
far this will be detrimental to Clang. If we are still in disagreement then 
perhaps we should move this discussion to cfe-dev? I want to make sure that we 
are all aligned on the overall direction and that we don't diverge too much 
from what was already discussed on cfe-dev.

Thanks in advance for your feedback,
-Andrzej

[1]
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/OptionUtils.cpp#L26
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L270
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L275
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L1145

[2] 
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/include/clang/Basic/Diagnostic.h#L1482-L1484

[3] http://lists.llvm.org/pipermail/cfe-dev/2020-June/065766.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

It sounds like what you want is a diagnostic library that's almost completely 
abstracted over the kinds of entities that can be stored in a diagnostic, 
including the definition of a source location.  I don't think that's 
incompatible with this patch; there's no need to suggest reversion.

Source locations, supplemental source ranges, and fix-its are all still 
meaningful concepts in the driver, it's just that a source location is, say, an 
offset into a particular argument rather than a location in the file.  So the 
challenge for you is to come up with a way to abstract this that doesn't 
significantly make things worse for Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Hi All,

I've just noticed this patch and realised that it takes `DiagnosticBuilder` in 
the direction tangential to what we proposed in [1]. I've just restarted our 
efforts with regard to refactoring these APIs and feel that it would be good to 
coordinate any future efforts in this area. I also want to make sure that we 
have a solution that works for everyone.

Basically, this patch makes `DiagnosticBuilder` more integrated with 
libclangBasic by assuming that every diagnostic contains `SourceLocation`s and 
`FixItHint`s (see the definition of `DiagnosticStorage`). However, that's not 
the case for driver diagnostics (i.e. diagnostics issued in libclangDriver). In 
[1] we proposed to create thin abstraction layers above all diagnostic classes 
that do not depend on `SourceLocation` (or anything else from libclangBasic). 
This patch makes `DiagnosticBuilder` _more_ dependant on `SourceLocation` and 
in this respect refactors `DiagnosticBuilder` in a direction opposite to what 
we proposed in [1].

Since this patch changes the `DiagnosticBuilder` API quite significantly, I am 
not entirely sure how to proceed with refactoring it (we need to make it 
independent of `SourceLocation` and any related classes). Originally, 
`DiagnosticBuilder` was a standalone class, but now we will have to refactor 
the following classes on top of `DiagnosticBuilder`:

- `DiagnosticStorage`
- `StreamingDiagnostic`
- `PartialDiagnostic`

One idea that immediately comes to mind is to split `DiagnosticStorage` as 
follows:

  struct DiagnosticStorage {
  // Storage that applies to all diagnostics
  // Original content, but without DiagRanges and FixItHints
  };
  
  struct LangDiagnosticStorage {
  // Storage that applies only to diagnostics that contain ranges and 
FixitHints - in practice these are language diagnostics
  SmallVector DiagRanges;
  SmallVector FixItHints;
  
  LangDiagnosticStorage() = default;
  }

How does this look? Sadly it will clutter this API, but I can't see any way 
around it. One other extreme alternative is to:

- revert this patch
- create a thin abstraction layer for `DiagnosticBuilder` independent of 
SourceLocation (as per our RFC [1])
- re-apply this patch (with some necessary modifications)

I appreciate that it is important to keep this API as clean and as lean as 
possible. However,

- it's required by libclangDriver
- we want to make libclangDriver independent of Clang (so that it can be used 
by Flang without the dependency on Clang).

Further refactoring will be required to achieve this. We sent 2 RFCs [1] [2] 
earlier this year to cfe-dev to discuss this, so I hope that this won't come as 
a surprise.

Do you have any suggestions?

CC @tra @rjmccall  @rsmith @yaxunl

Thank you,
Andrzej

[1] http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html
[2] http://lists.llvm.org/pipermail/llvm-dev/2020-June/141994.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7e561b62d2f2: [NFC] Refactor DiagnosticBuilder and 
PartialDiagnostic (authored by yaxunl).
Herald added a subscriber: dexonsmith.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Attr.h
  clang/include/clang/AST/CanonicalType.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclarationName.h
  clang/include/clang/AST/DependentDiagnostic.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/PartialDiagnostic.h
  clang/include/clang/Sema/DelayedDiagnostic.h
  clang/include/clang/Sema/Ownership.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Basic/Diagnostic.cpp
  clang/unittests/Basic/DiagnosticTest.cpp

Index: clang/unittests/Basic/DiagnosticTest.cpp
===
--- clang/unittests/Basic/DiagnosticTest.cpp
+++ clang/unittests/Basic/DiagnosticTest.cpp
@@ -74,7 +74,7 @@
 TEST(DiagnosticTest, diagnosticError) {
   DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
   new IgnoringDiagConsumer());
-  PartialDiagnostic::StorageAllocator Alloc;
+  PartialDiagnostic::DiagStorageAllocator Alloc;
   llvm::Expected> Value = DiagnosticError::create(
   SourceLocation(), PartialDiagnostic(diag::err_cannot_open_file, Alloc)
 << "file"
Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -40,8 +40,8 @@
 
 using namespace clang;
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   DiagNullabilityKind nullability) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ DiagNullabilityKind nullability) {
   StringRef string;
   switch (nullability.first) {
   case NullabilityKind::NonNull:
@@ -61,8 +61,8 @@
   return DB;
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   llvm::Error &) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ llvm::Error &) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
@@ -482,13 +482,15 @@
 
   CurDiagLoc = storedDiag.getLocation();
   CurDiagID = storedDiag.getID();
-  NumDiagArgs = 0;
+  DiagStorage.NumDiagArgs = 0;
 
-  DiagRanges.clear();
-  DiagRanges.append(storedDiag.range_begin(), storedDiag.range_end());
+  DiagStorage.DiagRanges.clear();
+  DiagStorage.DiagRanges.append(storedDiag.range_begin(),
+storedDiag.range_end());
 
-  DiagFixItHints.clear();
-  DiagFixItHints.append(storedDiag.fixit_begin(), storedDiag.fixit_end());
+  DiagStorage.FixItHints.clear();
+  DiagStorage.FixItHints.append(storedDiag.fixit_begin(),
+storedDiag.fixit_end());
 
   assert(Client && "DiagnosticConsumer not set!");
   Level DiagLevel = storedDiag.getLevel();
@@ -1141,13 +1143,13 @@
   return Target.IncludeInDiagnosticCounts();
 }
 
-PartialDiagnostic::StorageAllocator::StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::DiagStorageAllocator() {
   for (unsigned I = 0; I != NumCached; ++I)
 FreeList[I] = Cached + I;
   NumFreeListEntries = NumCached;
 }
 
-PartialDiagnostic::StorageAllocator::~StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::~DiagStorageAllocator() {
   // Don't assert if we are in a CrashRecovery context, as this invariant may
   // be invalidated during a crash.
   assert((NumFreeListEntries == NumCached ||
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -254,8 +254,8 @@
   }
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   TemplateName N) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ TemplateName N) {
   std::string NameStr;
   llvm::raw_string_ostream OS(NameStr);
   LangOptions LO;
@@ -268,20 +268,6 @@
   return DB << 

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

Okay.  This LGTM if you feel that the operator forwarders is the better 
approach.


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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 299044.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.

Add constructors to StreamingDiagnostic.


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

https://reviews.llvm.org/D84362

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Attr.h
  clang/include/clang/AST/CanonicalType.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclarationName.h
  clang/include/clang/AST/DependentDiagnostic.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/PartialDiagnostic.h
  clang/include/clang/Sema/DelayedDiagnostic.h
  clang/include/clang/Sema/Ownership.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Basic/Diagnostic.cpp
  clang/unittests/Basic/DiagnosticTest.cpp

Index: clang/unittests/Basic/DiagnosticTest.cpp
===
--- clang/unittests/Basic/DiagnosticTest.cpp
+++ clang/unittests/Basic/DiagnosticTest.cpp
@@ -74,7 +74,7 @@
 TEST(DiagnosticTest, diagnosticError) {
   DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
   new IgnoringDiagConsumer());
-  PartialDiagnostic::StorageAllocator Alloc;
+  PartialDiagnostic::DiagStorageAllocator Alloc;
   llvm::Expected> Value = DiagnosticError::create(
   SourceLocation(), PartialDiagnostic(diag::err_cannot_open_file, Alloc)
 << "file"
Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -40,8 +40,8 @@
 
 using namespace clang;
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   DiagNullabilityKind nullability) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ DiagNullabilityKind nullability) {
   StringRef string;
   switch (nullability.first) {
   case NullabilityKind::NonNull:
@@ -61,8 +61,8 @@
   return DB;
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   llvm::Error &) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ llvm::Error &) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
@@ -482,13 +482,15 @@
 
   CurDiagLoc = storedDiag.getLocation();
   CurDiagID = storedDiag.getID();
-  NumDiagArgs = 0;
+  DiagStorage.NumDiagArgs = 0;
 
-  DiagRanges.clear();
-  DiagRanges.append(storedDiag.range_begin(), storedDiag.range_end());
+  DiagStorage.DiagRanges.clear();
+  DiagStorage.DiagRanges.append(storedDiag.range_begin(),
+storedDiag.range_end());
 
-  DiagFixItHints.clear();
-  DiagFixItHints.append(storedDiag.fixit_begin(), storedDiag.fixit_end());
+  DiagStorage.FixItHints.clear();
+  DiagStorage.FixItHints.append(storedDiag.fixit_begin(),
+storedDiag.fixit_end());
 
   assert(Client && "DiagnosticConsumer not set!");
   Level DiagLevel = storedDiag.getLevel();
@@ -1141,13 +1143,13 @@
   return Target.IncludeInDiagnosticCounts();
 }
 
-PartialDiagnostic::StorageAllocator::StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::DiagStorageAllocator() {
   for (unsigned I = 0; I != NumCached; ++I)
 FreeList[I] = Cached + I;
   NumFreeListEntries = NumCached;
 }
 
-PartialDiagnostic::StorageAllocator::~StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::~DiagStorageAllocator() {
   // Don't assert if we are in a CrashRecovery context, as this invariant may
   // be invalidated during a crash.
   assert((NumFreeListEntries == NumCached ||
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -254,8 +254,8 @@
   }
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   TemplateName N) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ TemplateName N) {
   std::string NameStr;
   llvm::raw_string_ostream OS(NameStr);
   LangOptions LO;
@@ -268,20 +268,6 @@
   return DB << NameStr;
 }
 
-const PartialDiagnostic::operator<<(const PartialDiagnostic ,
-   TemplateName N) {
-  std::string NameStr;
-  llvm::raw_string_ostream 

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/PartialDiagnostic.h:66
+return *this;
+  }
+

yaxunl wrote:
> rjmccall wrote:
> > Why are these template operators necessary?  The LHS of the `<<` should 
> > convert to a base reference type just fine.
> There are lots of usage patterns expecting the derived class after streaming, 
> e.g.
> 
> ```
> void foo(PartialDiagnostic& PD);
> foo(PD << X << Y);
> ```
I see.  You could also just do this in the base operators by just making them 
templates, e.g.

```
template 
std::enable_if_t, const 
Diagnostic &>
operator<<(const Diagnostic , ...) {
}
```

That way you'd never have these forwarding problems — but it would be more 
boilerplate for each operator, so maybe it's a wash.


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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/Diagnostic.h:1065
+///
+class StreamableDiagnosticBase {
+public:

rjmccall wrote:
> I think I would prefer `StreamingDiagnostic` as the class name here.
renamed



Comment at: clang/include/clang/Basic/PartialDiagnostic.h:66
+return *this;
+  }
+

rjmccall wrote:
> Why are these template operators necessary?  The LHS of the `<<` should 
> convert to a base reference type just fine.
There are lots of usage patterns expecting the derived class after streaming, 
e.g.

```
void foo(PartialDiagnostic& PD);
foo(PD << X << Y);
```


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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 298368.
yaxunl marked an inline comment as done.
yaxunl added a comment.

Rename StreamableDiagnosticBase to StreamingDiagnostic.


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

https://reviews.llvm.org/D84362

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Attr.h
  clang/include/clang/AST/CanonicalType.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclarationName.h
  clang/include/clang/AST/DependentDiagnostic.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/PartialDiagnostic.h
  clang/include/clang/Sema/DelayedDiagnostic.h
  clang/include/clang/Sema/Ownership.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Basic/Diagnostic.cpp
  clang/unittests/Basic/DiagnosticTest.cpp

Index: clang/unittests/Basic/DiagnosticTest.cpp
===
--- clang/unittests/Basic/DiagnosticTest.cpp
+++ clang/unittests/Basic/DiagnosticTest.cpp
@@ -74,7 +74,7 @@
 TEST(DiagnosticTest, diagnosticError) {
   DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
   new IgnoringDiagConsumer());
-  PartialDiagnostic::StorageAllocator Alloc;
+  PartialDiagnostic::DiagStorageAllocator Alloc;
   llvm::Expected> Value = DiagnosticError::create(
   SourceLocation(), PartialDiagnostic(diag::err_cannot_open_file, Alloc)
 << "file"
Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -40,8 +40,8 @@
 
 using namespace clang;
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   DiagNullabilityKind nullability) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ DiagNullabilityKind nullability) {
   StringRef string;
   switch (nullability.first) {
   case NullabilityKind::NonNull:
@@ -61,8 +61,8 @@
   return DB;
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   llvm::Error &) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ llvm::Error &) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
@@ -481,13 +481,15 @@
 
   CurDiagLoc = storedDiag.getLocation();
   CurDiagID = storedDiag.getID();
-  NumDiagArgs = 0;
+  DiagStorage.NumDiagArgs = 0;
 
-  DiagRanges.clear();
-  DiagRanges.append(storedDiag.range_begin(), storedDiag.range_end());
+  DiagStorage.DiagRanges.clear();
+  DiagStorage.DiagRanges.append(storedDiag.range_begin(),
+storedDiag.range_end());
 
-  DiagFixItHints.clear();
-  DiagFixItHints.append(storedDiag.fixit_begin(), storedDiag.fixit_end());
+  DiagStorage.FixItHints.clear();
+  DiagStorage.FixItHints.append(storedDiag.fixit_begin(),
+storedDiag.fixit_end());
 
   assert(Client && "DiagnosticConsumer not set!");
   Level DiagLevel = storedDiag.getLevel();
@@ -1140,13 +1142,13 @@
   return Target.IncludeInDiagnosticCounts();
 }
 
-PartialDiagnostic::StorageAllocator::StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::DiagStorageAllocator() {
   for (unsigned I = 0; I != NumCached; ++I)
 FreeList[I] = Cached + I;
   NumFreeListEntries = NumCached;
 }
 
-PartialDiagnostic::StorageAllocator::~StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::~DiagStorageAllocator() {
   // Don't assert if we are in a CrashRecovery context, as this invariant may
   // be invalidated during a crash.
   assert((NumFreeListEntries == NumCached ||
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -254,8 +254,8 @@
   }
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   TemplateName N) {
+const StreamingDiagnostic ::operator<<(const StreamingDiagnostic ,
+ TemplateName N) {
   std::string NameStr;
   llvm::raw_string_ostream OS(NameStr);
   LangOptions LO;
@@ -268,20 +268,6 @@
   return DB << NameStr;
 }
 
-const PartialDiagnostic::operator<<(const PartialDiagnostic ,
-   TemplateName N) {
-  std::string NameStr;
-  

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, this looks a lot better.




Comment at: clang/include/clang/Basic/Diagnostic.h:1065
+///
+class StreamableDiagnosticBase {
+public:

I think I would prefer `StreamingDiagnostic` as the class name here.



Comment at: clang/include/clang/Basic/PartialDiagnostic.h:66
+return *this;
+  }
+

Why are these template operators necessary?  The LHS of the `<<` should convert 
to a base reference type just fine.


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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/PartialDiagnostic.h:51
+  : DiagID(DiagID) {
+Allocator = _;
+  }

tra wrote:
> Is there a particular reason to move field initialization into the body here 
> and in other constructors?
Allocator is now a member of the base class, therefore needs to be initialized 
directly. I could add ctor to the base class but I did not see too much benefit 
of doing that.


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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/include/clang/Basic/PartialDiagnostic.h:51
+  : DiagID(DiagID) {
+Allocator = _;
+  }

Is there a particular reason to move field initialization into the body here 
and in other constructors?


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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 298125.
yaxunl added a comment.

revised by John's comments.

Extracted common part of DiagnosticEngine and PartialDiagnostics as 
DiagnosticStorage.

Make member functions of the base class of DiagnosticBuilder and 
ParticalDiagnostics non-virtual.


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

https://reviews.llvm.org/D84362

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Attr.h
  clang/include/clang/AST/CanonicalType.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclarationName.h
  clang/include/clang/AST/DependentDiagnostic.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/PartialDiagnostic.h
  clang/include/clang/Sema/DelayedDiagnostic.h
  clang/include/clang/Sema/Ownership.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Basic/Diagnostic.cpp
  clang/unittests/Basic/DiagnosticTest.cpp

Index: clang/unittests/Basic/DiagnosticTest.cpp
===
--- clang/unittests/Basic/DiagnosticTest.cpp
+++ clang/unittests/Basic/DiagnosticTest.cpp
@@ -74,7 +74,7 @@
 TEST(DiagnosticTest, diagnosticError) {
   DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
   new IgnoringDiagConsumer());
-  PartialDiagnostic::StorageAllocator Alloc;
+  PartialDiagnostic::DiagStorageAllocator Alloc;
   llvm::Expected> Value = DiagnosticError::create(
   SourceLocation(), PartialDiagnostic(diag::err_cannot_open_file, Alloc)
 << "file"
Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -40,8 +40,9 @@
 
 using namespace clang;
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   DiagNullabilityKind nullability) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase ,
+   DiagNullabilityKind nullability) {
   StringRef string;
   switch (nullability.first) {
   case NullabilityKind::NonNull:
@@ -61,8 +62,8 @@
   return DB;
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   llvm::Error &) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , llvm::Error &) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
@@ -481,13 +482,15 @@
 
   CurDiagLoc = storedDiag.getLocation();
   CurDiagID = storedDiag.getID();
-  NumDiagArgs = 0;
+  DiagStorage.NumDiagArgs = 0;
 
-  DiagRanges.clear();
-  DiagRanges.append(storedDiag.range_begin(), storedDiag.range_end());
+  DiagStorage.DiagRanges.clear();
+  DiagStorage.DiagRanges.append(storedDiag.range_begin(),
+storedDiag.range_end());
 
-  DiagFixItHints.clear();
-  DiagFixItHints.append(storedDiag.fixit_begin(), storedDiag.fixit_end());
+  DiagStorage.FixItHints.clear();
+  DiagStorage.FixItHints.append(storedDiag.fixit_begin(),
+storedDiag.fixit_end());
 
   assert(Client && "DiagnosticConsumer not set!");
   Level DiagLevel = storedDiag.getLevel();
@@ -1140,13 +1143,13 @@
   return Target.IncludeInDiagnosticCounts();
 }
 
-PartialDiagnostic::StorageAllocator::StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::DiagStorageAllocator() {
   for (unsigned I = 0; I != NumCached; ++I)
 FreeList[I] = Cached + I;
   NumFreeListEntries = NumCached;
 }
 
-PartialDiagnostic::StorageAllocator::~StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::~DiagStorageAllocator() {
   // Don't assert if we are in a CrashRecovery context, as this invariant may
   // be invalidated during a crash.
   assert((NumFreeListEntries == NumCached ||
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -254,8 +254,8 @@
   }
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   TemplateName N) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , TemplateName N) {
   std::string NameStr;
   llvm::raw_string_ostream OS(NameStr);
   LangOptions LO;
@@ -268,20 +268,6 @@
   return DB << NameStr;
 }
 
-const PartialDiagnostic::operator<<(const PartialDiagnostic ,
-   TemplateName N) {
-  

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/Diagnostic.h:1086-1090
 /// Note that many of these will be created as temporary objects (many call
 /// sites), so we want them to be small and we never want their address taken.
 /// This ensures that compilers with somewhat reasonable optimizers will 
promote
 /// the common fields to registers, eliminating increments of the NumArgs 
field,
 /// for example.

rnk wrote:
> This comment documents that this object is intended to be small. Adding a 
> vtable grows it by one pointer, and the virtual methods practically ensure 
> that it will always be address-taken and will not be promoted from memory to 
> registers. I think you need to reconsider the design of this patch. I will 
> revert it for now. Please get approval from @rsmith or @rjmccall before 
> adding a vtable to this class gain.
I don't know that copying a DiagnosticBuilder is actually an important thing to 
support, but yes, this doesn't really seem like the right approach.  The bigger 
problem is that it still leaves an enormous amount of code duplication between 
the active diagnostic being built in the DiagnosticsEngine and the diagnostic 
being built in the PartialDiagnostic — it's basically the same storage concept, 
duplicated in two different places!  We should unify those two things, so that 
there's only one class implementing the storage of an active diagnostic.  There 
can then be a common object of that class in DiagnosticsEngine, which 
DiagnosticBuilder then stores a reference to, while PartialDiagnostic has its 
own copy.  And then it should be fairly straightforward to make the operators 
work with either.

Among other things, that might make it significantly more efficient to emit a 
diagnostic from a `PartialDiagnostic` — we might be able to just emit it from 
the internal storage instead of having to copy it into the engine, so that the 
engine's use of a common diagnostic object is just a storage optimization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: rjmccall, rnk.
rnk added inline comments.



Comment at: clang/include/clang/Basic/Diagnostic.h:1086-1090
 /// Note that many of these will be created as temporary objects (many call
 /// sites), so we want them to be small and we never want their address taken.
 /// This ensures that compilers with somewhat reasonable optimizers will 
promote
 /// the common fields to registers, eliminating increments of the NumArgs 
field,
 /// for example.

This comment documents that this object is intended to be small. Adding a 
vtable grows it by one pointer, and the virtual methods practically ensure that 
it will always be address-taken and will not be promoted from memory to 
registers. I think you need to reconsider the design of this patch. I will 
revert it for now. Please get approval from @rsmith or @rjmccall before adding 
a vtable to this class gain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.

In D84362#2283078 , @tra wrote:

> It's possible. Unfortunately it's only triggered by our internal tool and 
> it's hard to create a public reproducer for it. I'll debug and try to fix it 
> on Monday.

It appears the patch slightly increased stack use and caused a failure in code 
that was running with a limited stack space available.
AFAICT there are no issues with this patch per se and it's good to re-land, 
again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D84362#2283065 , @yaxunl wrote:

> I think this is probably a different issue. The issue reported in D84364 
>  was introduced by change in D84364 
> .

It's possible. Unfortunately it's only triggered by our internal tool and it's 
hard to create a public reproducer for it. I'll debug and try to fix it on 
Monday.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D84362#2283045 , @tra wrote:

> In D84362#2282992 , @yaxunl wrote:
>
>> The fix is for the change in D84364 . It 
>> has no effect on the change in this review. Are you sure the issue you saw 
>> is due to change in this review instead of change in D84364 
>> ?
>
> Pretty sure. We've bisected the failure specifically to commit 
> ee5519d323571c4a9a7d92cb817023c9b95334cd 
> , before 
>  D84364  landed.

I think this is probably a different issue. The issue reported in D84364 
 was introduced by change in D84364 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D84362#2282992 , @yaxunl wrote:

> The fix is for the change in D84364 . It has 
> no effect on the change in this review. Are you sure the issue you saw is due 
> to change in this review instead of change in D84364 
> ?

Pretty sure. We've bisected the failure specifically to commit 
ee5519d323571c4a9a7d92cb817023c9b95334cd 
, before  
D84364  landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D84362#2282965 , @tra wrote:

> In D84362#2282890 , @yaxunl wrote:
>
>> I have a fix for the issue reported in D84364 
>> . Would you like to try? Thanks.
>
> I can try it on the internal test that crashed with the patch. I've reopened 
> this review and will pick up the diff once you update it.

The fix is for the change in D84364 . It has 
no effect on the change in this review. Are you sure the issue you saw is due 
to change in this review instead of change in D84364 
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Artem Belevich via Phabricator via cfe-commits]

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

In D84362#2282890 , @yaxunl wrote:

> I have a fix for the issue reported in D84364 
> . Would you like to try? Thanks.

I can try it on the internal test that crashed with the patch. I've reopened 
this review and will pick up the diff once you update it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D84362#2279845 , @tra wrote:

> In D84362#2279688 , @tra wrote:
>
>> Apparently this patch triggers compiler crashes on some of our code. I'll 
>> try to create a reproducer, but it would be great to revert the patch for 
>> now.
>
> It's likely the same issue as the one reported in D84364 
> , but we probably triggered it independently.

I have a fix for the issue reported in D84364 
. Would you like to try? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D84362#2279688 , @tra wrote:

> Apparently this patch triggers compiler crashes on some of our code. I'll try 
> to create a reproducer, but it would be great to revert the patch for now.

It's likely the same issue as the one reported in D84364 
, but we probably triggered it independently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Apparently this patch triggers compiler crashes on some of our code. I'll try 
to create a reproducer, but it would be great to revert the patch for now.

  *** SIGSEGV (@0x7f68892d8fe8),  received by PID 8210 (TID 8225) on cpu 65; 
stack trace: ***
  PC: @ 0x7f686d8a169d  (unknown)  clang::Decl::getAvailability()
  @ 0x7f6712fad13b   1376  FailureSignalHandler()
  @ 0x7f696f1b69a0  34448  (unknown)
  @ 0x7f688c278c9a352  ShouldDiagnoseAvailabilityOfDecl()
  @ 0x7f688c27897d352  clang::Sema::DiagnoseAvailabilityOfDecl()
  @ 0x7f688c8bd3c7   1584  clang::Sema::DiagnoseUseOfDecl()
  @ 0x7f688c8cc1e8672  clang::Sema::BuildDeclarationNameExpr()
  @ 0x7f688c8c9e30192  clang::Sema::BuildDeclarationNameExpr()
  @ 0x7f688c8c2698   1280  clang::Sema::ActOnIdExpression()
  @ 0x7f688e9b95db   9136  clang::Parser::ParseCastExpression()
  @ 0x7f688e9b358b128  clang::Parser::ParseCastExpression()
  @ 0x7f688e9b9ebb   9136  clang::Parser::ParseCastExpression()
  @ 0x7f688e9b358b128  clang::Parser::ParseCastExpression()
  @ 0x7f688e9b1998112  
clang::Parser::ParseAssignmentExpression()
  @ 0x7f688e9b1832 64  clang::Parser::ParseExpression()
  @ 0x7f688e9bd6f0   4064  clang::Parser::ParseParenExpression()
  @ 0x7f688e9b6bb4   9136  clang::Parser::ParseCastExpression()
  @ 0x7f688e9b358b128  clang::Parser::ParseCastExpression()
  @ 0x7f688e9b1998112  
clang::Parser::ParseAssignmentExpression()
  @ 0x7f688e9c1285288  clang::Parser::ParseExpressionList()
  @ 0x7f688e9b4731   3088  
clang::Parser::ParsePostfixExpressionSuffix()
  @ 0x7f688e9bb92a   9136  clang::Parser::ParseCastExpression()
  @ 0x7f688e9b358b128  clang::Parser::ParseCastExpression()
  @ 0x7f688e9b1998112  
clang::Parser::ParseAssignmentExpression()
  @ 0x7f688e9b1832 64  clang::Parser::ParseExpression()
  @ 0x7f688ea43040240  clang::Parser::ParseReturnStatement()
  @ 0x7f688ea3cef0   1264  
clang::Parser::ParseStatementOrDeclarationAfterAttributes()
  @ 0x7f688ea3c5cf160  
clang::Parser::ParseStatementOrDeclaration()
  @ 0x7f688ea4490a   1264  
clang::Parser::ParseCompoundStatementBody()
  @ 0x7f688ea45df9272  
clang::Parser::ParseFunctionStatementBody()
  @ 0x7f688ea6a7b4960  clang::Parser::ParseFunctionDefinition()
  @ 0x7f688e9600bd   2880  clang::Parser::ParseDeclGroup()
  @ 0x7f688ea69634368  
clang::Parser::ParseDeclOrFunctionDefInternal()
  @ 0x7f688ea68c20672  
clang::Parser::ParseDeclarationOrFunctionDefinition()
  @ 0x7f688ea684ee768  clang::Parser::ParseExternalDeclaration()
  @ 0x7f688ea667a9368  clang::Parser::ParseTopLevelDecl()
  @ 0x7f688e93d2a2240  clang::ParseAST()
  @ 0x7f688fe079bd 80  clang::ASTFrontendAction::ExecuteAction()
  @ 0x7f688fe073b8144  clang::FrontendAction::Execute()
  @ 0x7f688fd1bdd6480  clang::ASTUnit::Parse()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGee5519d32357: [NFC] Refactor DiagnosticBuilder and 
PartialDiagnostic (authored by yaxunl).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Attr.h
  clang/include/clang/AST/CanonicalType.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclarationName.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/PartialDiagnostic.h
  clang/include/clang/Sema/Ownership.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Basic/Diagnostic.cpp

Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -40,8 +40,9 @@
 
 using namespace clang;
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   DiagNullabilityKind nullability) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase ,
+   DiagNullabilityKind nullability) {
   StringRef string;
   switch (nullability.first) {
   case NullabilityKind::NonNull:
@@ -61,8 +62,8 @@
   return DB;
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   llvm::Error &) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , llvm::Error &) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -254,8 +254,8 @@
   }
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   TemplateName N) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , TemplateName N) {
   std::string NameStr;
   llvm::raw_string_ostream OS(NameStr);
   LangOptions LO;
@@ -268,20 +268,6 @@
   return DB << NameStr;
 }
 
-const PartialDiagnostic::operator<<(const PartialDiagnostic ,
-   TemplateName N) {
-  std::string NameStr;
-  llvm::raw_string_ostream OS(NameStr);
-  LangOptions LO;
-  LO.CPlusPlus = true;
-  LO.Bool = true;
-  OS << '\'';
-  N.print(OS, PrintingPolicy(LO));
-  OS << '\'';
-  OS.flush();
-  return PD << NameStr;
-}
-
 void TemplateName::dump(raw_ostream ) const {
   LangOptions LO;  // FIXME!
   LO.CPlusPlus = true;
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -448,8 +448,8 @@
   llvm_unreachable("Invalid TemplateArgument Kind!");
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   const TemplateArgument ) {
+template 
+static const T (const T , const TemplateArgument ) {
   switch (Arg.getKind()) {
   case TemplateArgument::Null:
 // This is bad, but not as bad as crashing because of argument
@@ -502,6 +502,11 @@
   llvm_unreachable("Invalid TemplateArgument Kind!");
 }
 
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , const TemplateArgument ) {
+  return DiagTemplateArg(DB, Arg);
+}
+
 const ASTTemplateArgumentListInfo *
 ASTTemplateArgumentListInfo::Create(const ASTContext ,
 const TemplateArgumentListInfo ) {
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -3301,12 +3301,7 @@
   llvm_unreachable("Invalid access specifier!");
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   AccessSpecifier AS) {
-  return DB << getAccessName(AS);
-}
-
-const PartialDiagnostic ::operator<<(const PartialDiagnostic ,
-   AccessSpecifier AS) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , AccessSpecifier AS) {
   return DB << getAccessName(AS);
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11294,9 +11294,9 @@
   return *OMPTraitInfoVector.back();
 }
 
-const DiagnosticBuilder &
-clang::operator<<(const 

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Artem Belevich via Phabricator via cfe-commits]

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

This is a very nice cleanup. Thank you, Sam.


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

https://reviews.llvm.org/D84362

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

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 292319.
yaxunl retitled this revision from "[NFC] Add missing functions to 
PartialDiagnostic" to "[NFC] Refactor DiagnosticBuilder and PartialDiagnostic".
yaxunl edited the summary of this revision.
yaxunl added a comment.

Revised by Artem's comments. Extracted the common code of DiagnosticBuilder and 
PartialDiagnostic
and removed redundant code.


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

https://reviews.llvm.org/D84362

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Attr.h
  clang/include/clang/AST/CanonicalType.h
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/AST/DeclarationName.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/AST/TemplateName.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/PartialDiagnostic.h
  clang/include/clang/Sema/Ownership.h
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TemplateName.cpp
  clang/lib/Basic/Diagnostic.cpp

Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -40,8 +40,9 @@
 
 using namespace clang;
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   DiagNullabilityKind nullability) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase ,
+   DiagNullabilityKind nullability) {
   StringRef string;
   switch (nullability.first) {
   case NullabilityKind::NonNull:
@@ -61,8 +62,8 @@
   return DB;
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   llvm::Error &) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , llvm::Error &) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -254,8 +254,8 @@
   }
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   TemplateName N) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , TemplateName N) {
   std::string NameStr;
   llvm::raw_string_ostream OS(NameStr);
   LangOptions LO;
@@ -268,20 +268,6 @@
   return DB << NameStr;
 }
 
-const PartialDiagnostic::operator<<(const PartialDiagnostic ,
-   TemplateName N) {
-  std::string NameStr;
-  llvm::raw_string_ostream OS(NameStr);
-  LangOptions LO;
-  LO.CPlusPlus = true;
-  LO.Bool = true;
-  OS << '\'';
-  N.print(OS, PrintingPolicy(LO));
-  OS << '\'';
-  OS.flush();
-  return PD << NameStr;
-}
-
 void TemplateName::dump(raw_ostream ) const {
   LangOptions LO;  // FIXME!
   LO.CPlusPlus = true;
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -448,8 +448,8 @@
   llvm_unreachable("Invalid TemplateArgument Kind!");
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   const TemplateArgument ) {
+template 
+static const T (const T , const TemplateArgument ) {
   switch (Arg.getKind()) {
   case TemplateArgument::Null:
 // This is bad, but not as bad as crashing because of argument
@@ -502,6 +502,11 @@
   llvm_unreachable("Invalid TemplateArgument Kind!");
 }
 
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , const TemplateArgument ) {
+  return DiagTemplateArg(DB, Arg);
+}
+
 const ASTTemplateArgumentListInfo *
 ASTTemplateArgumentListInfo::Create(const ASTContext ,
 const TemplateArgumentListInfo ) {
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -3301,12 +3301,7 @@
   llvm_unreachable("Invalid access specifier!");
 }
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   AccessSpecifier AS) {
-  return DB << getAccessName(AS);
-}
-
-const PartialDiagnostic ::operator<<(const PartialDiagnostic ,
-   AccessSpecifier AS) {
+const StreamableDiagnosticBase ::
+operator<<(const StreamableDiagnosticBase , AccessSpecifier AS) {
   return DB << getAccessName(AS);
 }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++