In D70691#1775169, @erichkeane wrote:

I looked at the test failure, the REQUIRES line is necessary because the test calls emit-obj. I've asked Zahira to commit it to fix the bots.

Got other build fails.
Doesn't the LIT test require a:
// REQUIRES: x86-registered-target
command?

In D70691#1773611, @leonardchan wrote:

LGTM. Could you commit this soon to get the bots green again?

This might be easier to read the edit that I have made:
ksh-3.2$ git diff
diff --git a/clang/test/CodeGen/opt-record-1.c b/clang/test/CodeGen/opt-record-1.c
index 3f37e32..00a753d 100644

• a/clang/test/CodeGen/opt-record-1.c

+++ b/clang/test/CodeGen/opt-record-1.c
@@ -1,12 +1,12 @@
- RUN: %clang_cc1 %s -opt-record-file=t1.opt -fopenmp -emit-llvm-bc -o %t.bc
- RUN: %clang_cc1 -x ir %t.bc -opt-record-file %t.opt -fopenmp -emit-obj
+ RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 %s -O3 -opt-record-file=t1.opt -fopenmp -emit-llvm-bc -o %t.bc
+ RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -target-cpu x86-64 -O3 -x ir %t.bc -opt-record-file %t.opt -fopenmp -emit-obj
// RUN: cat %t.opt | FileCheck -check-prefix=CHECK %s

If you agree with the edit I will commit the change. Thanks.

Please see the edit in the list test. Fixed the build issue that occurred in the buildbot:
http://lab.llvm.org:8011/builders/clang-x86_64-debian-new-pass-manager-fast/builds/232/steps/test-check-all/logs/stdio

Fixed the indentation. Thanks.

Changed it. Thanks.

Thanks for the review. Please let me know if I have addressed everything.

After an (offline) review from @erichkeane (added now as a reviewer) , stripped the unnecessary code. Thanks.

In D65774#1619537, @rupprecht wrote:

Agree with all these comments -- I'm no expert on when and when not to std::move, but this is undoing changes I've made to fix buildbots on different compilers.

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.