Review anyone please? Thanks.

This patch is only to fix the compiler crash.
Would you mind commenting/reviewing this patch as it is a crash with (internal) impact?
There would still be another bug that needs fixing (would prefer to open another bug for it), for this case:
class __declspec(dllexport) foo {

foo(int x = 0);

};

In D84038#2179734, @mehdi_amini wrote:

In D84038#2179551, @zahiraam wrote:

I actually made an attempt to commit the patch using arc but not sure it made it. This was my last command: git commit --amend -a
Do I need to push now?
Thanks.

You can use arc land --revision D84038 or you can check what you're about to push: git fetch origin && git log ^origin/master HEAD ; if this is the commit you intend to push and the commit message is satisfying, then just push it.

If you don't have push permissions, you need to ask here for someone to land it for you.

I actually made an attempt to commit the patch using arc but not sure it made it. This was my last command: git commit --amend -a
Do I need to push now?
Thanks.

Can someone submit it please? Thanks.

Review please? Thanks.

In D84038#2165110, @beanz wrote:

Sadly I think @rnk isn't around. This is something I'd really like him to weigh in on. I assume there was a reason for doing this per-object file rather than for everything, and I don't have the context.

@hans Thanks for the feedback.
@riccibruno Thanks for the feedback.

In D79591#2092223, @hans wrote:

I'm sorry, but I still don't understand, and neither will anyone who reads the new code, I think. It needs an explanation.

@hans Thanks for looking at it. I think the updated patch is a better way to handle the crash.

@majnemer Any feedback? Thanks.

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.