[PATCH] D135099: [C2x] Implement support for nullptr and nullptr_t

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/PrettyPrinter.h:68
         SuppressDefaultTemplateArgs(true), Bool(LO.Bool),
-        Nullptr(LO.CPlusPlus11), Restrict(LO.C99), Alignof(LO.CPlusPlus11),
+        Nullptr(LO.CPlusPlus11), NullptrTypeInNamespace(LO.CPlusPlus),
+        Restrict(LO.C99), Alignof(LO.CPlusPlus11),
----------------
erichkeane wrote:
> Does Nullptr here not have to change?  Shouldn't we care to use the "nullptr" in C mode instead of '0' now?
Yes it should, but it's an NFC change. The only thing using `Nullptr` is the `APValue` pretty printer and I can't find a way to exercise that code path in C. That said, once we add support for `constexpr` in C, I believe we will be able to get into this path so we might as well handle it now.

If anyone can think of a test case that attempts to print a diagnostic on a pointer typed APValue in C, I'm happy to add it.


================
Comment at: clang/include/clang/AST/PrettyPrinter.h:201
+  /// Whether 'nullptr_t' is in namespace 'std' or not.
+  unsigned NullptrTypeInNamespace;
+
----------------
erichkeane wrote:
> Is there a reason this isn't a bitfield as well?
Nope, great catch! Think-o on my part. :-)


================
Comment at: clang/include/clang/Basic/TokenKinds.def:393
 CXX11_KEYWORD(noexcept              , 0)
-CXX11_KEYWORD(nullptr               , 0)
+CXX11_KEYWORD(nullptr               , KEYC2X)
 CXX11_KEYWORD(static_assert         , KEYMSCOMPAT|KEYC2X)
----------------
erichkeane wrote:
> Its a shame we have to do this... the CXX11_KEYWORD (and presumably the reverse) is used for 'future' diagnostics IIRC.  So not having this as a C2X_KEYWORD means we lose the 'future keyword' diagnostics, right?
Nope, we do the right thing now after Usman's changes. I'll add a test to demonstrate so we have better coverage.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2999
+      Self.Diag(SrcExpr.get()->getExprLoc(), diag::err_nullptr_cast)
+          << 0 /*nullptr to type*/ << DestType;
+      SrcExpr = ExprError();
----------------
shafik wrote:
> Curious why put the comment after? When we use `butprone-argument-comment` for function parameters we put them before. 
No real reason aside from muscle memory. I'll switch it up.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:8730
+  //   result also has that type.
+  if (LHSTy->isNullPtrType() && Context.hasSameType(LHSTy, RHSTy))
+    return ResTy;
----------------
shafik wrote:
> erichkeane wrote:
> > what does this do with the GNU ternary-thing?  
> Does this cover if one is a pointer with a null pointer value?
It behaves as expected; I've added additional test coverage to demonstrate that, good call!


================
Comment at: clang/lib/Sema/SemaExpr.cpp:12587
 
-    // Comparison of Objective-C pointers and block pointers against nullptr_t.
-    // These aren't covered by the composite pointer type rules.
-    if (!IsOrdered && RHSType->isNullPtrType() &&
-        (LHSType->isObjCObjectPointerType() || LHSType->isBlockPointerType())) {
+  // C++ [expr.eq]p4:
+  //   Two operands of type std::nullptr_t or one operand of type
----------------
shafik wrote:
> This was in the `if (getLangOpts().CPlusPlus)` block previously, did you mean to move it out?
Yup, moving it out is intentional. This now covers C2x 6.5.9p5 as well.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:12620
+
+  // Comparison of Objective-C pointers and block pointers against nullptr_t.
+  // These aren't covered by the composite pointer type rules.
----------------
shafik wrote:
> This was in the `if (getLangOpts().CPlusPlus)` block previously, did you mean to move it out?
Yes -- this was handling Objective-C++ behavior and now that C has nullptr we want it to cover Objective-C as well.


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

https://reviews.llvm.org/D135099