Thanks for the review.
I think and hope that I have responded to every issue you raised. Let me know if there are still pending issues.
Happy 4th!

A review please :-) Thanks.

And this patch actually fixes the bug. Thanks.

@rsmith I think I have made all the changes you have pointed out to. But please note that this new patch only impements an explicit AST representation of uuid in template arguments. It does not fix the bug for which this review was opened for.
I will take care of the bug in time. But before doing that I want to make sure that the changes required to give an explicit AST for uuid is correct.

There are still a few things I haven't addressed yet. Mostly because not sure there is another solution like getting rid of the map from StringRef to expr. The other issue is not adding new kind to ParsedAttr. There may be another way of doing it, but didn't look at it yet.
Meanwhile can you please let me know if you are happy with the current status of the implementation.

It would be nice to have a review for this year old (updated) patch. Thanks.

In D45978#1431057, @aaron.ballman wrote:

LGTM!

In D41950#1404899, @rnk wrote:

lgtm

Let's see if I have included every thing mentioned. Thanks.

In D45978#1392855, @aaron.ballman wrote:

It looks like the patch got mucked up somehow, I only see three testing files in the patch now?

In D45978#1387385, @aaron.ballman wrote:

Can you add tests for C mode as well, as it seems the behavior differs there.

That seems like a reasonable place to try, to me.

In D45978#1379901, @rnk wrote:

I'm still not sure this is the best place to make this change, but the functionality is important. There are still unaddressed comments (no need to check MSVCCompatibility, formatting), and I think once those are fixed we can land this.

Feedback please. Thanks.

Please advise.
Thanks.

Feedback on this please. Thanks.

Is this better? Thanks.

In D45978#1077051, @rnk wrote:

This shouldn't be conditional under -fms-compatibility, it's part of dllexport, which is already under -fms-extensions / -fdeclspec-extensions.

I don't think this is the right place to do this. Can it be done as part of processing the dllexport attribute or as part of the main storage class calculation?

Richard,
Please let me know if I have answered to all the issues you raised. Thanks.

In D43576#1016295, @majnemer wrote:

We should really, really avoid making this sort of change without first trying to desugar uuidof into a reference to a variable. That would solve a ton of problems, problems like this one.

This is the LLVM I get. The above test case is failing:

I think this breaks CodeGenCXX/dllimport-base.cpp and devirtualize-ms-dtor.cpp?

LGTM 2. It fixes PR32990.

Attached the test case.

Test case to add.

In D38803#1031304, @erichkeane wrote:

In D38803#1031303, @zahiraam wrote:

In D38803#1030905, @rnk wrote:

Can you check the base revision you used to upload the patch? I think there should just be new test case code now.

Yes. Can you please commit the test case?

I believe he's saying you should rebase, since an additional testcase has already been added.

Oops! I thought I had done it already. Sorry. On it.

In D38803#1030905, @rnk wrote:

Can you check the base revision you used to upload the patch? I think there should just be new test case code now.

Hopefully I have responded to the issues you have raised.
I have added the test case you have submitted, to make sure that an expression is generated.
Some of the error messages in the lit test have to be updated (they do match MSVC error messages).

In D43576#1016588, @rsmith wrote:

In D43576#1016561, @majnemer wrote:

Here's my thinking: the __uuidof expression literally declares a variable called _GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3 of type __s_GUID which is why it behaves the way it does: https://godbolt.org/g/74FY7U

This is an implementation detail leaking, though. no? Note that that is a reserved name.

I don't think it is reasonable to invent new semantics which are different from the MSVC ones because we find the MSVC ones inelegant.

I mostly agree, but my point is that this is *not* the MSVC semantics, it's merely an implementation detail that non-conforming code happens to be able to observe. Suppose that type_info objects were similarly accessible in MSVC by guessing their mangled names. Would you be arguing that we should inject variables for those too? (And note that it is *nearly* true that type_info objects work that way: https://godbolt.org/g/zByFFg -- but the parser gets confused somehow when you reference them.) The only difference I can see between these cases is that the reserved name used for the GUID case happens to not contain any ?s and @s, so happens to be utterable as an identifier.

We should not attempt to be compatible with the cases where MSVC's implementation details happen to leak into user-visible semantics.

What is the relative upside to a new kind of Decl? Better AST fidelity?

Yes, exactly. The same reason we don't desguar other things any more than we have to.

In D42968#1019198, @hans wrote:

I think you only uploaded the diff between this and the previous version. But the S.MarkVariableReferenced version LGTM. Do you want me to commit it for you?

Added a test case to check the suppression of the warning.

In D43576#1016295, @majnemer wrote:

We should really, really avoid making this sort of change without first trying to desugar uuidof into a reference to a variable. That would solve a ton of problems, problems like this one.

Adding test case.

Fixing the assert in the build.

Rebasing on rL315656 actually fixes the problem. No deleting destructor is generated. So adding the test case only to insure that is enough. The code of this release is already committed.

Added my patch to https://reviews.llvm.org/rL315656 .

See https://reviews.llvm.org/D40621

This code and the (updated) test case are all included in the review here:
https://reviews.llvm.org/D39063

I would appreciate feedback on this please! May be adding another reviewer if you guys don't have time? Thanks.

In D42968#1004673, @hans wrote:

Looks good to me. Do you have commit access or would you like me to commit it for you?