rsmith marked 2 inline comments as done.
rsmith added a comment.
In D77962#1978668 <https://reviews.llvm.org/D77962#1978668>, @rnk wrote:
> This code is very string-y. Should clang be parsing uuids into
> u32-u16-u16-u8[8] earlier, so that we can get the semantic form directly from
> the UuidAttr? Based on the test cases you had to pass, is there any reason
> that would be difficult?
I think that's a good idea, and should be reasonably straightforward. (The
CodeGen side of this, which also parses the UUID, is also stringy, as is the
mangling side, and probably none of these three should be.)
================
Comment at: clang/lib/AST/ExprConstant.cpp:3949
+ // Special-case reading from __uuidof expressions so we don't have to
+ // construct an APValue for the whole uuid.
+ if (LVal.Designator.isOnePastTheEnd()) {
----------------
aeubanks wrote:
> For my knowledge, why try to avoid constructing an APValue for the whole
> uuid? Is special casing this necessary?
I don't think it's incredibly important. It matters a little bit for a case
like:
```
constexpr MyGuid f(const GUID &g) {
return {g.Data1, g.Data2, g.Data3, g.Data4[0], g.Data4[1], ..., g.Data4[7]};
}
MyGuid g = f(__uuidof(IUnknown));
```
... where if we always created a complete `APValue`, we'd produce the complete
`APValue` 11 times. It also didn't seem to be any harder than always forming
the complete `APValue` for the GUID.
If we create a new kind of `Decl` to represent a UUID and store the `APValue`
there, we can remove nearly all the `CXXUuidofExpr` special casing in constant
evaluation and template arguments. That might actually be pretty
straightforward, and I want to do something very similar for template parameter
objects for C++20 class type non-type template parameters. But we don't need to
do that in order to fix this bug, so I'm mildly inclined to go with the special
case here and then remove this code again by a refactoring.
================
Comment at: clang/test/CodeGenCXX/microsoft-uuidof.cpp:37
// Make sure we can properly generate code when the UUID has curly braces on
it.
-GUID thing = __uuidof(Curly);
+GUID thing = (side_effect(), __uuidof(Curly));
// CHECK-DEFINE-GUID: @thing = global %struct._GUID zeroinitializer, align 4
----------------
rnk wrote:
> Do we need to add coverage of the constexpr-evaluated initializer codepath?
> Is it just a matter of updating the CHECKs, or do we reject this code now?
>
> If we reject, will that create a migration problem for users?
We still accept the old form of this code, but would constant initialize it
instead. Code generation for the constant initialization case is tested by
`GUID const_init` below. Support for `constexpr` GUID variables initialized by
`__uuidof` is covered in `test/SemaCXX/ms-uuid.cpp`.
I needed to change this code because we no longer generated a dynamic
initializer for this variable (because we can now constant-evaluate it
instead), and both this `CHECK` and the one below for the "static [sic]
initializer for thing" would fail.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77962/new/
https://reviews.llvm.org/D77962
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
