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@-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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
