[PATCH] D84362: [NFC] Add missing functions to PartialDiagnostic

2020-09-15 Thread Artem Belevich via Phabricator via cfe-commits 
tra added a comment.

In D84362#2275029 , @yaxunl wrote:

> 

Perhaps we could inherit `PartialDiagnostic` from `DiagnosticBuilder` base 
class. This would probably be the least invasive approach as it would preserve 
existing uses while allowing reuse of common `DiagnosticBuilder` functionality. 
That said, it would muddle the meaning of PartialDiagnostic even more -- is it 
the diagnostic itself, or a thing that produces the diagnostic?

To think of it, we actually don't need the `DiagnosticBuilderBase` per se, but 
rather a limited subset of operations for implementing freestanding 
`operator<<`.  So the base class for `DiagnosticBuilder` and 
`PartialDiagnostic` could be called `StreamableDiagnosticBase` and only provide 
those bits. This way the arrangement looks a bit less awkward. Both 
DiagnosticBuilder and PartialDiagnostic would inherit from it and would work 
with `operator<<(StreamableDiagnosticBase)`. We may need to convert  
StreamableDiagnosticBase results back to the original type.


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] Add missing functions to PartialDiagnostic

2020-09-15 Thread Yaxun Liu via Phabricator via cfe-commits 
yaxunl added a comment.

In D84362#2274884 , @tra wrote:

> In D84362#2274790 , @yaxunl wrote:
>
>> There are use patterns expecting `PartialDiagnosticInst << X << Y` to 
>> continue to be a `PartialDiagnostic&`, e.g.
>>
>>   PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);
>>
>> However if we derive PartialDiagnostic and DiagnosticBuilder from a base 
>> class DiagnosticBuilderBase which implements the `<<` operators, 
>> `PartialDiagnosticInst << X << Y` will become a `DiagnosticBuilderBase&`, 
>> then we can no longer write the above code.
>>
>> That's one reason I use templates to implement `<<` operators.
>>
>> Do we want to sacrifice this convenience?
>
> I don't think we have to. 
> AFAICT, virtually all such patterns (and there are only 20-ish of them in 
> total) are used with `EmitFormatDiagnostic(S.PDiag())` which could be adapted 
> to accept `DiagnosticBuilderBase` and `Sema::PDiag()` changed to return 
> `PartialDiagnosticBuilder` with no loss of convenience.

I saw lots of usage of PartialDiagnosticAt, which is defined at

https://github.com/llvm/llvm-project/blob/8c3d0d6a5f5a2a521c4cbae7acbad82a49e2a92f/clang/include/clang/Basic/PartialDiagnostic.h#L417

It requres a PartialDiagnostic as the second argument.

A typical use is like

https://github.com/llvm/llvm-project/blob/69f98311ca42127df92527b6fc3be99841a15f12/clang/lib/Sema/AnalysisBasedWarnings.cpp#L1741

If we use a base class DiagnosticBuilderBase to implement `<<` operators, 
`S.PDiag(diag::warn_unlock_but_no_lock) << Kind << LockName` becomes 
`DiagnosticBuilderBase&` and we can no longer write the above code.

There are ~100 uses like 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] Add missing functions to PartialDiagnostic

2020-09-15 Thread Artem Belevich via Phabricator via cfe-commits 
tra added a comment.

In D84362#2274790 , @yaxunl wrote:

> There are use patterns expecting `PartialDiagnosticInst << X << Y` to 
> continue to be a `PartialDiagnostic&`, e.g.
>
>   PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);
>
> However if we derive PartialDiagnostic and DiagnosticBuilder from a base 
> class DiagnosticBuilderBase which implements the `<<` operators, 
> `PartialDiagnosticInst << X << Y` will become a `DiagnosticBuilderBase&`, 
> then we can no longer write the above code.
>
> That's one reason I use templates to implement `<<` operators.
>
> Do we want to sacrifice this convenience?

I don't think we have to. 
AFAICT, virtually all such patterns (and there are only 20-ish of them in 
total) are used with `EmitFormatDiagnostic(S.PDiag())` which could be adapted 
to accept `DiagnosticBuilderBase` and `Sema::PDiag()` changed to return 
`PartialDiagnosticBuilder` with no loss of convenience.


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] Add missing functions to PartialDiagnostic

2020-09-15 Thread Yaxun Liu via Phabricator via cfe-commits 
yaxunl added a comment.

In D84362#2274315 , @aaron.ballman 
wrote:

> In D84362#2271585 , @tra wrote:
>
>> So, the idea here is to do some sort of duck-typing and allow DiagBuilder to 
>> work with both `DiagnosticBuilder` and `PartialDiagnostic`.
>>
>> What bothers me is that unlike `Diagnostic` `PartialDiagnostic` seems to be 
>> commingling functionality of the builder with that of the diagnostic itself. 
>> I'm not sure if that's by design or just happened to be that way.
>>
>> I think a better approach would be to refactor `PartialDiagnostic` and 
>> separate the builder functionality. That should make it possible to create a 
>> common diagnostic builder base class with Partial/Full diagnostic deriving 
>> their own builder, if needed.
>>
>> That said, I'm not that familiar with the diags. Perhaps  @rtrieu 
>> @aaron.ballman would have better ideas.
>
> I'm similarly a bit uncomfortable with adding the SFINAE magic to make this 
> work instead of making a base class that will work for either kind of 
> diagnostic builder. I'm adding @rsmith to see if he has opinions as well.

There are use patterns expecting `PartialDiagnosticInst << X << Y` to continue 
to be a `PartialDiagnostic&`, e.g.

  PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);

However if we derive PartialDiagnostic and DiagnosticBuilder from a base class 
DiagnosticBuilderBase which implements the `<<` operators, 
`PartialDiagnosticInst << X << Y` will become a `DiagnosticBuilderBase&`, then 
we can no longer write the above code.

That's one reason I use templates to implement `<<` operators.

Do we want to sacrifice this convenience?


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] Add missing functions to PartialDiagnostic

2020-09-15 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a reviewer: rsmith.
aaron.ballman added a subscriber: rsmith.
aaron.ballman added a comment.

In D84362#2271585 , @tra wrote:

> So, the idea here is to do some sort of duck-typing and allow DiagBuilder to 
> work with both `DiagnosticBuilder` and `PartialDiagnostic`.
>
> What bothers me is that unlike `Diagnostic` `PartialDiagnostic` seems to be 
> commingling functionality of the builder with that of the diagnostic itself. 
> I'm not sure if that's by design or just happened to be that way.
>
> I think a better approach would be to refactor `PartialDiagnostic` and 
> separate the builder functionality. That should make it possible to create a 
> common diagnostic builder base class with Partial/Full diagnostic deriving 
> their own builder, if needed.
>
> That said, I'm not that familiar with the diags. Perhaps  @rtrieu 
> @aaron.ballman would have better ideas.

I'm similarly a bit uncomfortable with adding the SFINAE magic to make this 
work instead of making a base class that will work for either kind of 
diagnostic builder. I'm adding @rsmith to see if he has opinions as well.


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] Add missing functions to PartialDiagnostic

2020-09-14 Thread Artem Belevich via Phabricator via cfe-commits 
tra added subscribers: aaron.ballman, rtrieu.
tra added a comment.

So, the idea here is to do some sort of duck-typing and allow DiagBuilder to 
work with both `DiagnosticBuilder` and `PartialDiagnostic`.

What bothers me is that unlike `Diagnostic` `PartialDiagnostic` seems to be 
commingling functionality of the builder with that of the diagnostic itself. 
I'm not sure if that's by design or just happened to be that way.

I think a better approach would be to refactor `PartialDiagnostic` and separate 
the builder functionality. That should make it possible to create a common 
diagnostic builder base class with Partial/Full diagnostic deriving their own 
builder, if needed.

That said, I'm not that familiar with the diags. Perhaps  @rtrieu 
@aaron.ballman would have better ideas.


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] Add missing functions to PartialDiagnostic

2020-09-08 Thread Yaxun Liu via Phabricator via cfe-commits 
yaxunl added a comment.

ping. this is needed by https://reviews.llvm.org/D84364


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] Add missing functions to PartialDiagnostic

2020-07-22 Thread Yaxun Liu via Phabricator via cfe-commits 
yaxunl updated this revision to Diff 279951.
yaxunl added a comment.

fix build failure


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/DeclarationName.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/TemplateBase.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/TemplateBase.cpp
  clang/lib/Basic/Diagnostic.cpp

Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -40,26 +40,6 @@
 
 using namespace clang;
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   DiagNullabilityKind nullability) {
-  StringRef string;
-  switch (nullability.first) {
-  case NullabilityKind::NonNull:
-string = nullability.second ? "'nonnull'" : "'_Nonnull'";
-break;
-
-  case NullabilityKind::Nullable:
-string = nullability.second ? "'nullable'" : "'_Nullable'";
-break;
-
-  case NullabilityKind::Unspecified:
-string = nullability.second ? "'null_unspecified'" : "'_Null_unspecified'";
-break;
-  }
-
-  DB.AddString(string);
-  return DB;
-}
 
 const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
llvm::Error &) {
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -26,6 +26,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
@@ -448,8 +449,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 +503,16 @@
   llvm_unreachable("Invalid TemplateArgument Kind!");
 }
 
+const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
+   const TemplateArgument ) {
+  return DiagTemplateArg(DB, Arg);
+}
+
+const PartialDiagnostic ::operator<<(const PartialDiagnostic ,
+   const TemplateArgument ) {
+  return DiagTemplateArg(DB, Arg);
+}
+
 const ASTTemplateArgumentListInfo *
 ASTTemplateArgumentListInfo::Create(const ASTContext ,
 const TemplateArgumentListInfo ) {
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11164,3 +11164,11 @@
 return DB << Section.Decl;
   return DB << "a prior #pragma section";
 }
+
+const PartialDiagnostic ::
+operator<<(const PartialDiagnostic ,
+   const ASTContext::SectionInfo ) {
+  if (Section.Decl)
+return DB << Section.Decl;
+  return DB << "a prior #pragma section";
+}
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1511,6 +1511,7 @@
   BaseDiag << Value;
   return Diag;
 }
+const static bool IsDiagBuilder = false;
   };
 
   /// Emit a diagnostic.
Index: clang/include/clang/Sema/ParsedAttr.h
===
--- clang/include/clang/Sema/ParsedAttr.h
+++ clang/include/clang/Sema/ParsedAttr.h
@@ -1040,34 +1040,26 @@
   ExpectedFunctionWithProtoType,
 };
 
-inline const DiagnosticBuilder <<(const DiagnosticBuilder ,
-   const ParsedAttr ) {
+template <
+typename DiagBuilderT,
+typename std::enable_if::type * = nullptr>
+inline const DiagBuilderT <<(const DiagBuilderT ,
+  const ParsedAttr ) {
   DB.AddTaggedVal(reinterpret_cast(At.getAttrName()),
   DiagnosticsEngine::ak_identifierinfo);
   return DB;
 }
 
-inline const PartialDiagnostic <<(const PartialDiagnostic ,
-   const ParsedAttr ) {
-  PD.AddTaggedVal(reinterpret_cast(At.getAttrName()),
-  DiagnosticsEngine::ak_identifierinfo);
-

[PATCH] D84362: [NFC] Add missing functions to PartialDiagnostic

2020-07-22 Thread Artem Belevich via Phabricator via cfe-commits 
tra added a comment.

FYI, the patch does not compile:

  In file included from llvm/repo/clang/lib/Parse/ParseStmtAsm.cpp:13:
  In file included from /llvm/repo/clang/include/clang/Parse/Parser.h:24:
  llvm/repo/clang/include/clang/Sema/Sema.h:1833:10: error: use of overloaded 
operator '<<' is ambiguous (with operand types 'const 
clang::Sema::SemaDiagnosticBuilder' and 'clang::QualType')
DB << T;
~~ ^  ~
  llvm/repo/clang/include/clang/AST/Type.h:7144:28: note: candidate function 
[with DiagBuilderT = clang::Sema::SemaDiagnosticBuilder, $1 = nullptr]
  inline const DiagBuilderT <<(const DiagBuilderT , QualType T) {
 ^
  llvm/repo/clang/include/clang/Sema/Sema.h:1508:41: note: candidate function 
[with T = clang::QualType]
  friend const SemaDiagnosticBuilder <<(
  ^
  llvm/repo/clang/include/clang/AST/TemplateBase.h:684:26: note: candidate 
function
  const DiagnosticBuilder <<(const DiagnosticBuilder ,


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] Add missing functions to PartialDiagnostic

2020-07-22 Thread Yaxun Liu via Phabricator via cfe-commits 
yaxunl created this revision.
yaxunl added a reviewer: tra.

PartialDiagnostic misses some functions compared to DiagnosticBuilder.

This patch adds missing functions to PartialDiagnostic so that can emit
all diagnostics that can be emitted by DiagnosticBuilder.


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/DeclarationName.h
  clang/include/clang/AST/NestedNameSpecifier.h
  clang/include/clang/AST/TemplateBase.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/lib/AST/ASTContext.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/Basic/Diagnostic.cpp

Index: clang/lib/Basic/Diagnostic.cpp
===
--- clang/lib/Basic/Diagnostic.cpp
+++ clang/lib/Basic/Diagnostic.cpp
@@ -40,26 +40,6 @@
 
 using namespace clang;
 
-const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
-   DiagNullabilityKind nullability) {
-  StringRef string;
-  switch (nullability.first) {
-  case NullabilityKind::NonNull:
-string = nullability.second ? "'nonnull'" : "'_Nonnull'";
-break;
-
-  case NullabilityKind::Nullable:
-string = nullability.second ? "'nullable'" : "'_Nullable'";
-break;
-
-  case NullabilityKind::Unspecified:
-string = nullability.second ? "'null_unspecified'" : "'_Null_unspecified'";
-break;
-  }
-
-  DB.AddString(string);
-  return DB;
-}
 
 const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
llvm::Error &) {
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -26,6 +26,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
@@ -448,8 +449,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 +503,16 @@
   llvm_unreachable("Invalid TemplateArgument Kind!");
 }
 
+const DiagnosticBuilder ::operator<<(const DiagnosticBuilder ,
+   const TemplateArgument ) {
+  return DiagTemplateArg(DB, Arg);
+}
+
+const PartialDiagnostic ::operator<<(const PartialDiagnostic ,
+   const TemplateArgument ) {
+  return DiagTemplateArg(DB, Arg);
+}
+
 const ASTTemplateArgumentListInfo *
 ASTTemplateArgumentListInfo::Create(const ASTContext ,
 const TemplateArgumentListInfo ) {
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -11164,3 +11164,11 @@
 return DB << Section.Decl;
   return DB << "a prior #pragma section";
 }
+
+const PartialDiagnostic ::
+operator<<(const PartialDiagnostic ,
+   const ASTContext::SectionInfo ) {
+  if (Section.Decl)
+return DB << Section.Decl;
+  return DB << "a prior #pragma section";
+}
Index: clang/include/clang/Sema/ParsedAttr.h
===
--- clang/include/clang/Sema/ParsedAttr.h
+++ clang/include/clang/Sema/ParsedAttr.h
@@ -1040,34 +1040,26 @@
   ExpectedFunctionWithProtoType,
 };
 
-inline const DiagnosticBuilder <<(const DiagnosticBuilder ,
-   const ParsedAttr ) {
+template <
+typename DiagBuilderT,
+typename std::enable_if::type * = nullptr>
+inline const DiagBuilderT <<(const DiagBuilderT ,
+  const ParsedAttr ) {
   DB.AddTaggedVal(reinterpret_cast(At.getAttrName()),
   DiagnosticsEngine::ak_identifierinfo);
   return DB;
 }
 
-inline const PartialDiagnostic <<(const PartialDiagnostic ,
-   const ParsedAttr ) {
-  PD.AddTaggedVal(reinterpret_cast(At.getAttrName()),
-  DiagnosticsEngine::ak_identifierinfo);
-  return PD;
-}
-
-inline const DiagnosticBuilder <<(const DiagnosticBuilder ,
-   const ParsedAttr *At) {
+template <
+typename DiagBuilderT,
+typename std::enable_if::type * = nullptr>
+inline const DiagBuilderT