- User Since
- May 27 2014, 6:39 AM (368 w, 2 d)
Add a test to verify MarkFileIncludedFromModule does |= isImport instead of = isImport.
Test when a non-modular header *is* visible through a module import (mostly for completeness).
Tue, Jun 15
Split out smaller parts as separate reviews.
Fri, Jun 4
Thank you for fixing this, Markus.
Apr 6 2021
Mar 31 2021
Looks good to me. Have a few clarification questions.
Nov 30 2020
I'm not familiar with this code, so my review can be pretty flawed. Current fix feels more like a workaround for getPrimaryContext() behavior (that it can make a decl visible). On the surface it looks like getPrimaryContext() shouldn't have drastic side-effects. Do you know if making EnumConstantDecl visible is critical for retrieving a valid primary context?
Oct 30 2020
I believe the fix isn't controversial and doesn't require Richard's attention.
Oct 29 2020
Was the assertion triggered on existing test suite or on some other code?
Based on the description the added assertion seems to be sensible. And adding duplicate Decl pointers doesn't seem to be a goal. The change looks good to me, I still want to go through parent revisions to get more context.
Oct 19 2020
Sep 29 2020
Sep 24 2020
Thanks for the reviews!
Sep 22 2020
Sep 17 2020
Tweak stats' names and descriptions.
Sep 16 2020
Looks good to me. I would wait a few days before committing in case other reviewers have comments but not too long as you can address extra comments post-commit.
Sep 14 2020
Noticed that the warning and the fix-it might not work well with pragmas suppressing diagnostic and with header guards. But it's not a regression and I don't think it is worth improving these use cases preemptively.
Sep 11 2020
Sep 1 2020
Here is a live view-stats.py http://188.8.131.52:9000/ It will be up for some time. I'll add some screenshots for the case when this preview goes down.
Aug 31 2020
Aug 25 2020
Thanks for the review.
Tweak the error text.
Aug 20 2020
Thanks for the review.
Aug 14 2020
Overall, looks like a reasonable approach to solve inode reuse. The problem with filenames is that they might not be canonicalized and the same file can be known by different filenames. I'm trying to think through the consequences in the following scenarios:
- same name but file content has changed;
- different names but refer to the same file.
Aug 12 2020
Aug 6 2020
Jul 30 2020
Rebased the change to make sure it still works.
Jul 23 2020
Jul 22 2020
Jul 17 2020
My concern about this approach is that it doesn't seem to be composable and propagating specific frontend actions to building modules looks ad-hoc and error-prone. Specifically, from the composition perspective what should we do when both indexing and fix-it are enabled? We can claim they should be mutually exclusive but don't know if it is applicable in general case. From the perspective of propagating other frontend actions to building modules I'm wondering if we need to have custom handling for other actions or not. Personally, I haven't checked that yet.
Jul 15 2020
Didn't get into details but overall GenModuleActionWrapper approach looks pretty complicated. Haven't tried to accomplish the same myself, so cannot say if such complexity is warranted or not. What happens with fixits in modules when you don't have GenModuleActionWrapper? How does it work with modules disabled? Also looks like there are no tests for incomplete umbrella warning when modules are disabled.
Jul 14 2020
Jul 9 2020
Jul 1 2020
With this change ninja clean works without errors for me (also regular ninja, ninja install). Approving from the functional perspective only, don't know CMake good enough to say if it can benefit from any improvements.
Jun 29 2020
Seems like this change causes ninja clean to fail with the error
Jun 9 2020
Also suggested clang-format linter changes make the test useless as the order of imports is important.
May 29 2020
Thanks for the review, Jonas!
May 28 2020
May 20 2020
There is nothing in header maps preventing from using relative paths. For example, I was able to run one of the tests with relative paths like
May 19 2020
The issue with this change is that it claims to add functionality that exists already. I.e., the test is passing without the change. The confusing part might be that even if DirectoryLookup::LookupFile doesn't find a file for relative destination immediately, it updates MappedName which is used by HeaderSearch::LookupFile. And I haven't tested it but looks like the change violates that header search paths are tested in order. So when we have -I A -I B.hmap -I C instead of processing A, B.hmap, C in that order, we can end up with A, B.hmap, A, B.hmap, C.
May 14 2020
Thanks everyone for reviews.
May 13 2020
Squash the commits, so that reviewers can review the entire change.
May 12 2020
Make compatibility mode accept correct types per James' Y Knight helpful suggestion.
May 6 2020
Patch for the suggested compatibility flag is available at https://reviews.llvm.org/D79511
May 4 2020
Agree that is a mistake in NSItemProvider API. The solution I offer is not to revert the change but to add cc1 flag -fcompatibility-qualified-id-block-type-checking and to make the driver for Darwin platforms to add this flag. Thus developers using Apple SDKs will see the old behavior and won't need to change their code. While everybody else will use clang with correct type checking. If any other platforms provide APIs relying on the old type checking, the solution for them is to tweak the driver.
Apr 24 2020
Apr 16 2020
Apr 10 2020
The added test fails on macOS. For example, in http://green.lab.llvm.org/green/job/clang-stage1-RA/8611/
Apr 7 2020
Apr 3 2020
Thanks for the review.
Mar 31 2020
Mar 27 2020
There should be no error for blockWithParam = genericBlockWithParam; because blockWithParam is called with I * and it is safe to substitute genericBlockWithParam. Basically you have
Mar 16 2020
Mar 12 2020
Mar 6 2020
What use case are you trying to support? Currently the added test headermap_relpath.cpp is passing without the changes to HeaderSearch.cpp, so it's not entirely clear what problem it should address.