Hello everyone.
@martong : this version of patch reorders only fields and friends. No method reordering is done (I can check if it works, though, and add the feature).
@davide: Unfortunately, I was not able to reproduce the problem on Linux. Could you please provide a link to a buildbot failure or some environment description so I can reproduce the issue or, at least, take a look?

Accidentally noticed about this review via cfe-commits. @NoQ, the change in the ExprEngine looks a bit dangerous to me. Could you please check?

Hi Balazs,
I have only two nits. Otherwise, the patch is OK and can be committed without additional approval after the comments are fixed. Thank you!

Hello Zoltán,

Whoops, sorry Balázs. Didn't look at the review author :(

Hello Gabor,

Hi Gabor,

Hi Balázs,

Looks good, thank you!

Hi Bevin,

LGTM!

Thank you!

Hi Gabor!

Hi Rafael,

Hi Rafael,

Hello Gabor,

You can find my comments inline.

I meant that we can use this approach for testImport() too.

This revision seems to be already committed in rC269693, without Differential Revision set.

LGTM too, thank you!
Do you need someone to commit this for you?

There are some results for clang and gcc max value for x86 and x64.
Source code:

const unsigned long long SIZE_MAX = (unsigned long long)(unsigned long)(-1);
const unsigned long long size = SIZE_MAX/2;
char arr[size+1];

Compiler output:

% g++ -c cast-comp.cpp -m32
cast-comp.cpp:6:16: error: size of array ‘arr’ is negative
 char arr[size+1];
                ^
% clang++-6.0 -c cast-comp.cpp -m32
% g++ -c cast-comp.cpp -m32        
cast-comp.cpp:6:16: error: size of array ‘arr’ is negative
 char arr[size+1];
                ^
% g++ -c cast-comp.cpp
cast-comp.cpp:6:16: error: size of array ‘arr’ is negative
 char arr[size+1];
                ^
% clang++-6.0 -c cast-comp.cpp
cast-comp.cpp:6:10: error: array is too large (9223372036854775808 elements)
char arr[size+1];
         ^~~~~~

So, clang accepts indices > SIZE_MAX/2 for x86.
For arr[size], only clang-x64 fails with error.
I think this means that we need to use LongLongTy as index type, not SizeType. @NoQ, what do you think?

I think, it is overkill to test all possible combinations of the options, we should come up with something better here.

I agree with that. I think we need to test just import pairs {/*From*/no_option, /*To*/no_option}, {option_1, option1}, {option_2, option_2}, ...{option_n, option_n}.
Another option is to just turn -fno-delayed-template-parsing -fno-ms-compatibility for ASTImporter tests like it is done in some unit tests, but I'm not sure it's a correct solution.

Hello Balázs!

Looks good to me, but the approval from AST code owners is required, I think.

Hm. Should we test -fms-compatibility in addition to -fdelayed-template-parsing?

Ok, I got it, thank you!

Hi Gabor,

Hi Bevin,

Hi Gabor,
Could you add a test for TSK_Undeclared as well?

LGTM with a nit.

LGTM, thanks!

(Sorry, accepted accidentially).

Hi Gabor! I have a question.

Nice!

LGTM. Aaron, could you please confirm that AST changes are fine?

So, we fail to add injected name to a CXXRecordDecl that has a described class template? Nice catch! LGTM.

Hello Gabor,

This is a nice extension of D16063.

Hello Rafael,

Ping.

Hello Gabor!

Hi Gabor,

Closed with rC332256.

In D46353#1086456, @martong wrote:

should we add this new declaration to the redeclaration chain like we do it for RecordDecls?

I think, on a long term we should. Otherwise we could loose e.g. C++11 attributes which are attached to the forward declaration only.
However, I'd do that as a separate commit, because that would require some independent changes and tests, also other decl kinds like ClassTemplateSepcializationDecl may be affected as well by that.

Gentle ping.

Add forgotten context.

After a number of attempts of Twine'ifying all Code samples, I decided to restore the initial state of this code.

Looks good!

Sorry, two more nits.

Hi Rafael! Please find my comments inline.

Hi Rafael! I like the change.

LGTM!

Hi Gabor,

I see,t hank you. Please feel free to submit a patch - it seems like you already have a nice test case that shows the difference between two import options.

Hello Peter! This looks almost OK, just some minor formatting issues.

Hi Rafael,

Hello Gabor,

Transform AST instead.
I don't remove analyzer guys from reviewers because the test is still for the analyzer.
@rsmith Feel free to correct me if I'm doing something wrong.

Malformed Herald rule, sorry.

Hi Henry. I had a quick look at the patch, here are some remarks.

Hi all. I'm already here, invited by the Herald - just had no time to take a look (will change the script to add me as a reviewer). But thank you anyway! :)

Confirming LGTM, no objections. Thank you!

Add a test for CFG dump; replace static_cast with an initialization.
No test failures on check-all were observed.

E->getFoundDecl().getDecl() can be null when a member expression does not involve lookup. (Note, it may involve a lookup in case of a using directive which refers to a member function in a base class template.)

Yes, a pretty weird example. Unfortunately, they are pretty common for XTU.

Looks good to me as well. Thank you!

Mostly LG.

This LGTM, but could you please add a test?

Hi Artem. Cool patch!