User Details
- User Since
- Jul 7 2012, 3:08 PM (559 w, 6 d)
Fri, Mar 17
lgtm, thanks!
This doesn't appear to behave as expected for PP_CacheFailure or PP_ScanFile without PP_CacheSuccess. Looks like that will still cache. Should probably just assert that's not the computed policy.
Thu, Mar 16
lgtm
Mon, Mar 13
LTGM, thanks!
Tue, Mar 7
Also handle module.map.
Forgot that Filename is actually a path, so call llvm::sys::path::filename.
Feb 1 2023
Jan 31 2023
Thanks for fixing this. LGTM.
Jan 17 2023
Jan 12 2023
Jan 10 2023
Jan 4 2023
Nov 8 2022
lgtm
lgtm.
Yeah, I agree with this. lgtm.
lgtm, thanks for fixing this.
I'm not aware of anyone using this mode, but please wait for responses from Google and Meta people to verify that.
Oct 26 2022
lgtm once D136624 lands.
Oct 24 2022
This looks reasonable. Have you measured the performance impact of this change?
Oct 19 2022
lgtm.
Oct 18 2022
Looks good with the suggestion to split the change.
Oct 12 2022
lgtm
Oct 11 2022
lgtm. Although I wonder if long term we should add something to the option tablegen file for this case?
Ah, I hit the same thing recently, thanks.
lgtm
lgtm
Oct 6 2022
lgtm
Sep 22 2022
lgtm
lgtm with formatting fixed.
Sep 21 2022
I think this is fine, but I need to think more about the ModuleDepCollector.cpp change.
lgtm.
Aug 30 2022
I'm fine with this change, but do we actually have a backwards compatibility policy anywhere in Clang? Would be good to know what range of SDKs a compiler release is expected to support.
Jun 15 2022
Jun 14 2022
Jun 6 2022
Mar 8 2022
Feb 14 2022
lgtm.
Feb 11 2022
This does require build systems to be able to totally replace a command line rather than just add to it, but it seems like that's a requirement unless we add a way to modify the command line to Clang. lgtm.
Feb 3 2022
lgtm. I agree that testing this isn't really necessary given the difficulty of even constructing one.
lgtm
Feb 2 2022
- Fixed documentation typo.
- Made missing '{' diagnostic more clear.
- Use ModuleMapParser::skipUntil.
Feb 1 2022
Drive-by comment on the docs; otherwise this sounds awesome; as long as else is easy to add later this SGTM (I'll let others do the code review).
(Although, if else {} and else requires {} would be easy to add now/soon, I feel like it'd be worth it. Modelling an else-requires chain by hand would be quite error-prone, and it might be annoying to stage the adoption separately...)
Add testing of empty blocks and nested blocks and make the missing { error clearer.
Is it possible to reference external module map from requires block? I mean that in one context the module has some extra requirements but in a different context doesn't have them.
Jan 27 2022
Jan 26 2022
Jan 25 2022
lgtm
Jan 24 2022
Jan 21 2022
Once these issues are fixed this should be fine.
Jan 5 2022
Dec 14 2021
Dec 2 2021
Can we not consider a modulemap used when we load an AST that depends on that modulemap? It's possible this breaks if the module only includes textual headers though. It really feels like we should have a single place where we actually know if a module is used or not. Long term I would really like to separate modulemap parsing from Module creation, which would be a great place to actually do this.
Dec 1 2021
There were two issues for Windows.
Nov 18 2021
I think this makes sense to do given that you need to explicitly opt into inferred modules anyway. This isn't implicitly finding module maps, as we already have a module map with framework module *.
lgtm
Nov 9 2021
Nov 5 2021
lgtm
Oct 28 2021
lgtm
Looks good. We shouldn't be checking _WIN32 just for path style questions.
Oct 27 2021
Looks reasonable. Edges with weight 0 shouldn't matter anyway.
Jul 27 2021
Here's a version that actually works (python 3, not sure if it's valid in 2), although I would much prefer we not write to the source directory during a build.
Also, this python script just doesn't work. It's missing a sys import, a "w" flag, and a new line after each write. It also dumps the output into the source directory. Was this actually tested?
Jul 26 2021
For future reference this was very difficult to merge into external changes. It looks like you resorted this at the same time as renaming it, and that messes up git's rename logic.
Jun 3 2021
lgtm.
lgtm
lgtm with a few minor changes.
May 26 2021
I think this is looking good, but would like either @dexonsmith, @akyrtzi, or someone else familiar with this area to take a look. Only other comment I have is that the new functions you add should use the current LLVM naming convention.
Apr 21 2021
lgtm.
Apr 16 2021
I think this is fine given that we already have a copy constructor with deep copy semantics.
lgtm
lgtm
lgtm
Mar 23 2021
Thanks for the cleanup. Code makes more sense now.
Mar 4 2021
LGTM
Looks fine to me, but it would be good for a Swift person to look at this patch.
Feb 25 2021
LGTM
Feb 16 2021
Needs an example in the "Creating new Command Line Option" section, but with that it looks good.
Feb 4 2021
Jan 28 2021
Jan 26 2021
Jan 14 2021
LGTM.
LGTM.
LGTM.
LGTM.
LGTM with the comment.
LGTM.
LGTM.
LGTM.