[PATCH] D149650: Give NullabilityKind a printing operator<<

Thu May 4 10:41:49 PDT 2023

sammccall added a comment.

In D149650#4316138 <https://reviews.llvm.org/D149650#4316138>, @aaron.ballman wrote:

> 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?

This can't be added downstream, the type needs to provide these.
ADL extension points like operators need to be in namespace `clang`[1] so that ADL will find them. If non-owners of the type define these, this ends up in violating ODR when two such non-owners are linked together. Hacking around this for the `operator<<` symbol itself (e.g. putting it inline in anonymous namespaces) results in ODR violations in templates that call them (in this case, gtest). Even defining them centrally in a separate header is risky, including/not including the header produces similar violations.

There's never going to be a hard need for this - having an ergonomic way to print things that composes with the rest of our infra is useful but it's always possible to live without it.
If we don't want such clang-as-a-library features upstream that's OK. We can define our own `NullabilityKind` and marshal between them - I need to be able to answer "why are we hacking around clang rather than improving it" in code review :-)

[1] (or `llvm`, or `::`, or `::testing` - these all have the same issues).

>>> `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).

Thanks - will do!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149650