erichkeane marked an inline comment as done. erichkeane added a comment. In D73967#1861073 <https://reviews.llvm.org/D73967#1861073>, @rsmith wrote:
> Have you considered how this would interact with our other language > extensions? Can we form a vector of `_ExtInt(N)`? A `_Complex _ExtInt(N)`? I > expect for some of that to just fall out of the implementation, but we should > have documentation and test coverage either way. > > I don't see any test coverage for `_Atomic(_ExtInt(N))`, which may not fall > out from the other work because (if memory serves) atomic lowering doesn't go > through the normal `ConvertTypeForMem` path and instead deals with padding > itself. > > I would like to see your documentation cover the calling convention used when > passing/returning these types by value. We can do a std::vector or any of the standard containers with no problems. Extended-vector types don't really make sense for non-powers-of-two (plus have some odd gotchas when it comes to vectors of i1 for example), so I've added a test that shows that this is rejected. _Complex _ExtInt(N) is rejected when parsing _Complex. It doesn't seem to make sense to me to support them, so I've added a test that shows it is invalid. Do you disagree? _Atomic seems to be broken (atomic memory access size mus be byte-sized), but I'll continue to work on it to update this patch further. I've also asked a coworker to better describe the calling convention so that we can add that text to the language-extensions. Thanks! ================ Comment at: clang/lib/AST/ItaniumMangle.cpp:3472 + BW = T->getNumBits(); + mangleIntegerLiteral(getASTContext().UnsignedIntTy, BW); +} ---------------- rsmith wrote: > This is not a valid vendor-extension mangling. There are two choices here, > per the current scheme: > > 1) mangle as a type, using (lowercase) `u` followed by a source-name, such as > `u9_ExtInt17` / `u10_UExtInt17` > 2) mangle as a type qualifier, using (capital) `U` followed by a source-name > and optional template-args, such as `U7_ExtIntILi17EEi` / `U7_ExtIntILi17EEj` > > Neither of these gives a particularly nice demangling. If WG14 seems likely > to accept the proposal, you should register a proper mangling as part of the > Itanium ABI. Well shucks, I was so close :) Looks like I'm just missing the last E/i from the second one. The second seems consistently changable with the one below (for a dependent version), so I think I'm stuck with that. It DOES show up a little weird unfortunately, but at least now it demangles. I'll ask about an official mangling mechanism for ItaniumABI. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73967/new/ https://reviews.llvm.org/D73967 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
