[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Andrey Ali Khan Bolshakov via Phabricator via lldb-commits]

bolshakov-a added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:3036
     if (!std::is_trivially_destructible<T>::value) {
-      auto DestroyPtr = [](void *V) { static_cast<T *>(V)->~T(); };
-      AddDeallocation(DestroyPtr, Ptr);
+      auto DestroyPtr = [](void *V) { ((T *)V)->~T(); };
+      AddDeallocation(DestroyPtr, (void *)Ptr);
----------------
erichkeane wrote:
> bolshakov-a wrote:
> > erichkeane wrote:
> > > This change is weird... what is going on here?
> > Here is not very beautiful attempt to workaround const-ness of `TemplateArgument::V::Value` pointer passed here from the added `TemplateArgument` constructor. The change in this line isn't acually needed and made only for consistence with the next line, I think. Alternatively, I can
> > 1) refactor `addDestruction` and `AddDeallocation` to work with pointers to constants, or
> > 2) add `const_cast` to `AddDeallocation` call in the next line, or
> > 3) make `TemplateArgument::V::Value` pointer non-const.
> > 
> > I'm biased to the first variant.
> I'd lean towards #3, it ends up being consistent with the rest of the things here.  #1 is interesting, but that results in these functions violating const-correctness.
I understand that calling the destructor on a reference to `const` looks strange, but it is reasonable: even constants should be destroyed.


================
Comment at: clang/include/clang/AST/TemplateBase.h:88
+    /// so cannot be dependent.
+    UncommonValue,
+
----------------
shafik wrote:
> erichkeane wrote:
> > I definitely hate the name here... Just `Value` makes a bit more sense, but isn't perfectly accurate.  Perhaps `NonTypeValue`?  But that is a little redundant.  `Uncommon` here is just strange and not particularly descriptive. 
> This catch all `UncommonValue` feels artificial. Maybe we need a separate out the cases into more granular cases like `Float` etc....
@erichkeane, it looks strange, I agree. Even just `CommonValue` sounds better for me (but my English is far from fluent). Maybe, `ArbitraryValue`?

@shafik, your suggestion would move this PR far enough from the original Richard's commit. And I'd prefer to merge `Declaration`, `Integral`, and `NullPtr` kinds into that is currently called `UncommonValue` rather than to repeat here various `APValue` kinds.


================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1670
+  case TemplateArgument::UncommonValue:
+    if (ValueDecl *D = getAsArrayToPointerDecayedDecl(
+            TA.getUncommonValueType(), TA.getAsUncommonValue())) {
----------------
erichkeane wrote:
> Has microsoft implemented this yet?  Can we see what they chose to do and make sure we match them? 
Experimentally, I've made me sure that MSVC produces the same mangled names for the cases provided in the `mangle-ms-templates` test as in the test. But there are problems with references to subobjects: some cases are explicitly unsupported in `mangleTemplateArgValue`, and some cases produce a magled name different from what MSVC does. Should it be fixed in this PR, or may be delayed?


================
Comment at: clang/lib/AST/TemplateBase.cpp:619
+  case TemplateArgument::UncommonValue: {
+    // FIXME: We're guessing at LangOptions!
+    SmallString<32> Str;
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > It's probably okay enough, but this is now the third instance of adding the same bug to this helper method -- maybe we should fix that instead?
> Agreed, seems to me we should do the work NOW to just wire in the lang-opts to this function.
The problem here is because this function is called from a stream insertion operator, and there isn't obviously any way to pass 3rd parameter into it without switching it into an ordinary function.


================
Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+    // FIXME: Visit value.
+    break;
----------------
aaron.ballman wrote:
> Any particular reason this isn't being handled now?
I need some guidance here. Which characters are allowed in the USR? Could the mangling algorithm from `CXXNameMangler::mangleValueInTemplateArg` be moved into some common place and reused here?


================
Comment at: clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp:204
+#if __cplusplus == 201703L
+  // cxx17-error at -3 {{non-type template argument refers to subobject '(int *)1'}}
+#endif
----------------
shafik wrote:
> Shouldn't this be an error b/c it is a temporary? What is `(int*)1` a subobject of?
This PR doesn't change C++17 mode diagnostics. Btw, in C++20 mode, this is acceptable template argument.


================
Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:12
+using F1 = Float<1.0f>;
+using F1 = Float<2.0f / 2>;
 
----------------
shafik wrote:
> I believe this is IFNDR the template-heads are functionally equivelent but not equivelent: https://eel.is/c++draft/temp.class#general-3
I don't think so, because `<1.0f>` and `<2.0f / 2>` are template argument lists and not template heads. Non-type template arguments may be written as arbitrary expressions which are converted before determining template argument equivalence. Expression result values are important.


================
Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:56
+using CF = ComplexFloat<1.0f + 3.0fi>;
+using CF = ComplexFloat<3.0fi + 1.0f>;
 
----------------
shafik wrote:
> Can we add an enum example e.g.:
> 
> ```
> enum E{ E1, E2};
> template <E> struct SE {};
> using IPE = SE<E1>;
> using IPE = SE<E2>;
> 
> ```
What for? Enumerators as non-type template arguments are allowed since C++98, AFAIK. And this test is about changes in C++20.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140996