aaron.ballman added a comment.

In D149650#4315211 <https://reviews.llvm.org/D149650#4315211>, @sammccall wrote:
> In D149650#4312389 <https://reviews.llvm.org/D149650#4312389>, @aaron.ballman 
> wrote:
>
>> I guess I'm not seeing much motivation for this change. We already have 
>> `clang::getNullabilitySpelling()` and `const StreamingDiagnostic 
>> &clang::operator<<(const StreamingDiagnostic &DB, DiagNullabilityKind 
>> nullability)` and now we're adding a third way to get this information. If 
>> this is just for debug/testing purposes, can we use existing debug 
>> formatters to convert the enumeration value into the enumerator name instead?
>
> We're using NullabilityKind in our dataflow-based null-safety clang-tidy 
> check <https://github.com/google/crubit/tree/main/nullability> that we hope 
> to upstream when it works.

Ah, good to know! I think it would be reasonable to add this functionality once 
there's some agreement on adding the clang-tidy check.

> (The idea is to use `_Nullable` and `_Nonnull` annotations on API boundaries 
> to gradually type pointers, and to provide a verification clang-tidy check 
> and libraries to infer annotations for existing code. The actual clang-tidy 
> check wrapper isn't in that repo yet, but it's trivial).
>
> It would be convenient if e.g. EXPECT_THAT(someVectorOfNK, 
> ElementsAre(Nullable, Unspecified)) 
> <https://github.com/google/crubit/blob/main/nullability/type_nullability_test.cc>
>  printed something useful when it fails, if we could write OS << NK and get 
> Unspecified rather than OS << getSpelling(NK) and get 
> _Unspecified_Nullability, etc.
> This doesn't concern clang core (ha, there are no unit tests...) but there's 
> no reasonable way to do this downstream without using a different type.

Hmmm, but does this need to be added to `Specifiers.h` instead of being kept 
packaged up with clang-tidy? For things like AST matchers, we usually ask that 
the matcher remain local to the clang-tidy check unless the same matcher is 
needed in multiple checks. This feels somewhat similar, despite it not being 
related to AST matching -- if the only need for this functionality exists out 
of the clang tree, can we keep it out of the clang tree until we find more 
needs?

>> `StreamingDiagnostic &clang::operator<<`
>
> This actually wants the user-visible spelling, so I think it can just use 
> getSpelling... if I make that change we're back to just two implementations 
> :-)

Oh, I really like those changes! Feel free to land that as an NFC change if 
you'd like (the addition of quotes is a bug fix, but not really a functional 
change).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149650

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

Reply via email to