- User Since
- Aug 15 2014, 7:40 AM (162 w, 2 d)
Fri, Sep 22
Wed, Sep 20
Tue, Sep 19
Tue, Sep 5
Aug 11 2017
Aug 10 2017
Aug 2 2017
Aug 1 2017
Jul 28 2017
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 :-)
Jul 27 2017
Jul 25 2017
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.
Jul 24 2017
Jul 17 2017
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.
Jul 9 2017
Thinking more about this, on Windows, is there a strong reason to default to a different libc by default on Windows?
Jul 6 2017
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.
Jun 30 2017
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...
Jun 29 2017
LGTM! Thanks Graydon
Jun 26 2017
Update patch after reviewer suggestions!
Jun 23 2017
Jun 22 2017
Any reason why this doesn't contain a testcase?
Jun 20 2017
New patch addressing comments from reviewers.
Jun 19 2017
Jun 6 2017
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?
Jun 2 2017
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.
Jun 1 2017
May 24 2017
Makes sense. LGTM!
May 23 2017
Updated the patch after Richard's comments. Removed the assertion but bail out early if the module is already visible.
May 22 2017
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...
May 19 2017
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.
May 17 2017
May 11 2017
May 10 2017
May 8 2017
May 3 2017
Apr 27 2017
Apr 26 2017
Thanks for your patience! LGTM with a minor comment below.
Thanks Alex. LGTM
Apr 25 2017
Apr 24 2017
Apr 20 2017
Update the patch after Richard's suggestions
Apr 17 2017
Apr 14 2017
@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.
Apr 13 2017
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.