Hi Boris,

Thanks Akira.

Hi Sean,

One more comment: please also fix names of local variables, member variables and function parameters. See https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly. Yes, there is precedence for violations in other places in this file, but that doesn't mean we should keep adding those :-)

In D34030#821282, @MontyKutyi wrote:

Added test for the fix.

In D34030#819699, @MontyKutyi wrote:

As I can see the clang/test contains a lot of different simple tests, but for testing this I think it is not enough to run the clang with some arguments on a specific input. So I should create a new executable which uses the postorder mode of the RecursiveASTVisitor. Am I right or is there another preferred way for doing this?

If there's a precedence for a flag name there, it's nice to be compatible here. You don't necessarily need to use the same name as the backend.

Hi Peter,

Hi Strahinja,

Could something like _WIN32 && _MSC_VER make this better? That would allow us to differentiate between the various environments. It's kinda icky, but, does make sense in a round about way.

Thinking more about this, on Windows, is there a strong reason to default to a different libc by default on Windows?

@v.g.vassilev will this also benefit from ContentCache::getBuffer improvements from D33275? I guess if getBuffer transparently handles pointing to right buffer we won't need this change?

That sounds even better. Thanks @v.g.vassilev

We're hitting a very similar problem to this one, which I think this patch will likely fix. I can't come up with a reproducible testcase either. If you can write one better yet, but I'm ok with this as is. LGTM.

Tested it locally, we actually need all these changes on top of D31778.

Hi @v.g.vassilev, sorry for the delay.

Digging into this a bit more, I think the root of the problem is that the ND->isExternallyVisible() call in LookupResult::isHiddenDeclarationVisible is wrong. Redeclaration lookup should never find hidden enumerators in C, because they do not have linkage (C11 6.2.2/6). (The same is true in C++, but I don't know whether we can apply the same thing there too, due to the different

merging rules.) A function foo in some source file should not conflict with an enumerator foo in a non-imported module, for instance.

The linkage of an enumerator should probably be VisibleNoLinkage, and isHiddenDeclarationVisible should probably be checking hasExternalFormalLinkage...

LGTM! Thanks Graydon

Update patch after reviewer suggestions!

Any reason why this doesn't contain a testcase?

Addressing comments!

New patch addressing comments from reviewers.

LGTM

About the tests and using --target=aarch64: you're right that there should nothing be ARM specific here, but it is just that for Aarch64 it will show "half" IR types which I preferred, while it looks like x86 way of dealing with is to convert it and work on i16 types. Perhaps I need tests for both?

Hi,

Yes, but that does not canonicalize the path. For instance, it won't handle doubled-up slashes or symlinks.

I'm unaware of any other API that canonicalizes the path, which is what users of this API are going to expect.

Hi Aaron,

LGTM!

Hi Sjoerd,

Hi Akira,

Hi Richard,

Makes sense. LGTM!

Updated the patch after Richard's comments. Removed the assertion but bail out early if the module is already visible.

Any idea why we're hitting this issue in the first place? The error that gets cleaned up is reported at some point before? Seems to me that we're going to fail to update the timestamp but continue as nothing happened, I wonder how many other issues this might trigger...

Update the patch to address @rsmith comments and rebase

I think the problem is that we don't take module visibility into account when doing redefinition checking for enumerators. Instead of passing through this flag, we should probably just ignore hidden declarations when checking for a redefinition of an enumerator.

Hi Vassil,

Hi Sean,

Thanks for your patience! LGTM with a minor comment below.

Thanks Alex. LGTM

@rsmith ping!

Ping!

Ping

Update the patch after Richard's suggestions

Ping!

@v.g.vassilev your testcase is interesting but it's unrelated to this specific improvement. The error message it's triggered by (a) the decl name not being found because kROOTD_OPEN is hidden, (b) type transforms looks for an alternative (c) Sema::diagnoseMissingImport is called and produces the error you're seeing. I can fix this one too once we decide what we consider a good diagnostic here, let's iterate in http://bugs.llvm.org/show_bug.cgi?id=32670

Updated the patch after Richard's comments:

Does this cause us to deserialize the SLocEntry for every FileID referenced by a RawComment? That would seem pretty bad.

@rsmith ping!

Done in r290134

r291466 & r291517

Done way back in r248932

Updating: already done in r298175

I ran this through IACA a few years ago, and didn't prove to be any better.

I don't have plans to work on this any time soon. Abandoning.