[PATCH] D66831: [ObjC] Fix type checking for qualified id block parameters.

vsapsai added a comment. In D66831#1650015 , @thakis wrote: > We're getting a bunch of errors looking like > `../../AppsListViewController.m:11:37: error: incompatible block pointer > types assigning to 'void (^)(__strong id)' from 'void > (^)(AppCollec

[PATCH] D66907: [Modules] Fix rebuilding an updated module for each of its consumers.

vsapsai marked an inline comment as done. vsapsai added inline comments. Comment at: clang/lib/Serialization/ModuleManager.cpp:211 -if (!getModuleCache().tryToDropPCM(NewModule->FileName)) - FileMgr.invalidateCache(NewModule->File); return OutOfDate; ---

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

vsapsai added a comment. For the record, there was another change regarding the delayed typos in `clang::Sema::~Sema()`: D62648 [Sema][Typo] Fix assertion failure for expressions with multiple typos. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.

vsapsai added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.

vsapsai updated this revision to Diff 218794. vsapsai added a comment. Herald added a subscriber: ributzka. - Rebase the patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 Files: clang/include/clang/Lex/DirectoryLookup.h clang/lib/Lex/He

[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.

vsapsai marked 3 inline comments as done. vsapsai added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:888 bool InUserSpecifiedSystemFramework = false; -bool HasBeenMapped = false; +bool CurrentInHeaderMap = false; bool IsFrameworkFoundInDir = false

[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.

vsapsai updated this revision to Diff 219194. vsapsai added a comment. - Add a test for unused absolute path in a header map; simplify code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 Files: clang/include/clang/Lex/DirectoryLookup.h cla

[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.

vsapsai marked 4 inline comments as done. vsapsai added a comment. Updated the code. Hope it is easier to understand now. Comment at: clang/lib/Lex/HeaderSearch.cpp:908-909 +if (IsMapped) + *IsMapped = CurrentInHeaderMap || HasBeenMapped; + dexonsmit

[PATCH] D66696: [ObjC generics] Fix not inheriting type bounds in categories/extensions.

vsapsai added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66696/new/ https://reviews.llvm.org/D66696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36853: [Parser] Correct initalizer typos before lambda capture type is deduced.

vsapsai created this revision. This is the same assertion as in https://reviews.llvm.org/D25206 that is triggered when RecordLayoutBuilder tries to compute the size of a field (for capture "typo_boo" in the test case) whose type hasn't been deduced. The fix is to add CorrectDelayedTyposInExpr cal

[PATCH] D36853: [Parser] Correct initalizer typos before lambda capture type is deduced.

vsapsai updated this revision to Diff 111716. vsapsai added a comment. Address review comments. Moved `CorrectDelayedTyposInExpr` according to review comment and added mentioned test case. Removed the assertion as it didn't catch the expression ParenListExpr 0x7f8297060e40 'NULL TYPE' `-Typ

[PATCH] D36853: [Parser] Correct initalizer typos before lambda capture type is deduced.

vsapsai added a comment. Thanks for review, Alex. I've requested commit access and plan to commit the change myself to make sure everything works as expected. https://reviews.llvm.org/D36853 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D36853: [Parser] Correct initalizer typos before lambda capture type is deduced.

This revision was automatically updated to reflect the committed changes. Closed by commit rL311480: [Parser] Correct initalizer typos before lambda capture type is deduced. (authored by vsapsai). Changed prior to commit: https://reviews.llvm.org/D36853?vs=111716&id=112201#toc Repository: rL

[PATCH] D37089: [Sema] Error out early for tags defined inside an enumeration.

vsapsai created this revision. This fixes PR28903 by avoiding access check for inner enum constant. We are performing access check because one enum constant references another and because enum is defined in CXXRecordDecl. But access check doesn't work because FindDeclaringClass doesn't expect more

[PATCH] D37089: [Sema] Error out early for tags defined inside an enumeration.

vsapsai added a comment. I have also considered adding tests for structs and unions defined inside an enumeration but decided it doesn't add much value, given the code is working for all TagDecl. Another option I had in mind is having a note pointing to outer enum. But the nesting is done in t

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

vsapsai added a comment. Think I'll need to make another review pass but here are my comments so far: - Why are you adding ODR hash support for `RecordDecl` and not `TagDecl`? We already have support for `EnumDecl`, so `TagDecl` seems like a good candidate to cover both. Honestly, I don't know

[PATCH] D82118: [clang][module] Improve incomplete-umbrella warning

vsapsai added a comment. 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. Comment at: clang/lib/Lex/PPLexerCh

[PATCH] D82118: [clang][module] Improve incomplete-umbrella warning

vsapsai accepted this revision. vsapsai added a comment. This revision is now accepted and ready to land. 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. Repository: rG LLVM Gith

[PATCH] D86895: [Modules] Add stats to measure performance of building and loading modules.

vsapsai planned changes to this revision. vsapsai added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:60 + +ALWAYS_ENABLED_STATISTIC(NumCompileModule, "Number of compiled modules."); + aprantl wrote: > `NumCompiledModules`? Sounds good to m

[PATCH] D86895: [Modules] Add stats to measure performance of building and loading modules.

vsapsai updated this revision to Diff 292658. vsapsai added a comment. Tweak stats' names and descriptions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86895/new/ https://reviews.llvm.org/D86895 Files: clang/lib/Frontend/CompilerInstance.cpp

[PATCH] D86895: [Modules] Add stats to measure performance of building and loading modules.

vsapsai added a comment. In D86895#2288262 , @tschuett wrote: > Where is the test? `ALWAYS_ENABLED_STATISTIC` has its own unit tests. And tests that use added stats are in another revision in the stack D86896 . Reposit

[PATCH] D86895: [Modules] Add stats to measure performance of building and loading modules.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc4bacc3c9b33: [Modules] Add stats to measure performance of building and loading modules. (authored by vsapsai). Repository: rG LLVM Github Monore

[PATCH] D86895: [Modules] Add stats to measure performance of building and loading modules.

vsapsai added a comment. Thanks for the reviews! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86895/new/ https://reviews.llvm.org/D86895 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://l

[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.

vsapsai added a comment. Thanks for the review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80263/new/ https://reviews.llvm.org/D80263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

[PATCH] D80263: [HeaderSearch] Fix processing #import-ed headers multiple times with modules enabled.

This revision was automatically updated to reflect the committed changes. Closed by commit rG7ac737e56bee: [HeaderSearch] Fix processing #import-ed headers multiple times with modules… (authored by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.

vsapsai updated this revision to Diff 287749. vsapsai added a comment. Tweak the error text. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84458/new/ https://reviews.llvm.org/D84458 Files: clang/include/clang/Basic/DiagnosticLexKinds.td clang/

[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.

This revision was automatically updated to reflect the committed changes. Closed by commit rG8839e278ffca: [Modules] Improve error message when cannot find parent module for submodule… (authored by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.

vsapsai marked an inline comment as done. vsapsai added a comment. Thanks for the review. Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:700 + "no module named '%0' %select{found|in '%2'}1, " + "expected parent module to be defined before the submodule">; def er

[PATCH] D86895: [Modules] Add stats to measure performance of building and loading modules.

vsapsai created this revision. vsapsai added reviewers: bruno, rsmith, Bigcheese. Herald added subscribers: llvm-commits, ributzka, dexonsmith, jkorous, hiraditya. Herald added projects: clang, LLVM. vsapsai requested review of this revision. Measure amount of high-level or fixed-cost operations

[PATCH] D88329: [objc] Consolidate ObjC name mangle code to AST

vsapsai added inline comments. Comment at: clang/lib/CodeGen/CGObjCMac.cpp:929 - /// \param[out] NameOut - The return value. - void GetNameForMethod(const ObjCMethodDecl *OMD, -const ObjCContainerDecl *CD, Note that the removed method is

[PATCH] D86895: [Modules] Add stats to measure performance of building and loading modules.

vsapsai added a comment. In D86895#2340548 , @thakis wrote: > Looks like the test is still failing on windows: > http://45.33.8.238/win/26155/step_11.txt > > Ptal, and revert for now if it takes a while to fix. Reverted. Have you seen similar problems on

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. In D109632#3000709 , @rmaz wrote: > The speedup is coming from reducing the number of times > `Sema::addMethodToGlobalList` is called when looking up methods loaded from > modules. From what I can see each serialized module will

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. In D109632#3006520 , @rmaz wrote: > The case we have is more like: > > .m -> A -> long list of partially shared deps -> Foundation >-> B -> long list of partially shared deps -> Foundation >-> C -> long list of

[PATCH] D110123: [Proof of concept] Serialize fewer transitive methods in `METHOD_POOL`.

vsapsai created this revision. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We are already making an effort to skip selectors that have methods only from other modules. But in edge cases we ke

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. Looks like in `METHOD_POOL` we don't need the transitive closure for performance reasons. And we are kinda trying to store only data the module owns > // Only write this selector if it's not in an existing AST or something > // changed. But even if a single method i

[PATCH] D110123: [Proof of concept] Serialize fewer transitive methods in `METHOD_POOL`.

vsapsai updated this revision to Diff 374016. vsapsai added a comment. Add a [failing] test case that checks handling methods from transitive modules. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110123/new/ https://reviews.llvm.org/D110123 Files

[PATCH] D110123: [Proof of concept] Serialize fewer transitive methods in `METHOD_POOL`.

vsapsai updated this revision to Diff 374017. vsapsai added a comment. Simplify the test and make it less sensitive to what "method" clang selects to use. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110123/new/ https://reviews.llvm.org/D110123

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. In D109632#3012647 , @rmaz wrote: >> What folks are thinking about writing less in METHOD_POOL? > > I prefer the idea of it, but I think the `ReadMethodPoolVisitor` also has to > be changed for this to work. When it finds a selec

[PATCH] D110280: [modules] Fix IRGen assertion on accessing ObjC ivar inside a method.

vsapsai created this revision. vsapsai added reviewers: rsmith, bruno, jansvoboda11. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. When have ObjCInterfaceDecl with the same name in 2 different modules, hitting the assertion > Assert

[PATCH] D110287: [modules] While merging ObjCInterfaceDecl definitions, merge them as decl contexts too.

vsapsai created this revision. vsapsai added reviewers: rsmith, bruno, jansvoboda11. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. While working on https://reviews.llvm.org/D110280 I've tried to merge decl contexts as it seems to be

[PATCH] D110280: [modules] Fix IRGen assertion on accessing ObjC ivar inside a method.

vsapsai added inline comments. Comment at: clang/lib/AST/DeclObjC.cpp:81-84 lookup_result R = lookup(Id); for (lookup_iterator Ivar = R.begin(), IvarEnd = R.end(); Ivar != IvarEnd; ++Ivar) { +if (auto *ivar = dyn_cast((*Ivar)->getCanonicalDecl())) --

[PATCH] D110452: [modules] Fix tracking ObjCInterfaceType decl when there are multiple definitions.

vsapsai created this revision. vsapsai added reviewers: bruno, rsmith, arphaman. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. With the old approach we were updating `ObjCInterfaceType.Decl` to the last encountered definition. But du

[PATCH] D110453: [modules] Update visibility for merged ObjCInterfaceDecl definitions.

vsapsai created this revision. vsapsai added reviewers: bruno, bnbarham. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. We keep using the first encountered definition and need to take into account visibility from subsequent definition

[PATCH] D110452: [modules] Fix tracking ObjCInterfaceType decl when there are multiple definitions.

vsapsai added a comment. This is a clean-up before https://reviews.llvm.org/D110453 Comment at: clang/lib/AST/DeclObjC.cpp:608 - // Make the type point at the definition, now that we have one. - if (TypeForDecl) -cast(TypeForDecl)->Decl = this; Tracking a

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. In D109632#3013620 , @dexonsmith wrote: > In D109632#3013523 , @vsapsai wrote: > >> 2. Serialize only methods owned by the current module (and change >> `ReadMethodPoolVisitor` appropria

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. In D109632#3021644 , @rmaz wrote: > @vsapsai just checking on where we are with this one. Is the current plan to > investigate an approach only serializing the methods declared in each module, > or are we good to go ahead with t

[PATCH] D110280: [modules] Fix IRGen assertion on accessing ObjC ivar inside a method.

vsapsai added a comment. In D110280#3016911 , @rjmccall wrote: > Hmm. It sounds like we misbehave if we're working with a non-canonical ivar. > While it does seem preferable in general for lookups done after merging > modules to return the canonical i

[PATCH] D110280: [modules] Fix IRGen assertion on accessing ObjC ivar inside a method.

vsapsai updated this revision to Diff 375771. vsapsai added a comment. Rebase and add a test case with `always_inline`. Interaction between `always_inline` function and `testAccessBitField` has revealed that we cannot avoid having expressions referencing decls from "wrong" module. So returning a

[PATCH] D110123: [Proof of concept] Serialize fewer transitive methods in `METHOD_POOL`.

vsapsai updated this revision to Diff 376084. vsapsai added a comment. Visit all required modules and don't serialize methods from other modules. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110123/new/ https://reviews.llvm.org/D110123 Files: c

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. I've updated D110123 to be the way I was planning it to be and I was testing locally with it. So far, with this change the speedup over a baseline is ~20%, though measurements aren't super rigorous. I.e., no multiple runs to warm up th

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. In D109632#3033489 , @rmaz wrote: > In D109632#3032381 , @vsapsai wrote: > >> What's interesting, I was able to trigger more diagnostic. Specifically, I >> got warnings about `length` amb

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai created this revision. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Currently we are using `HeaderSearch` state from building a module and if an invisible submodule imports a non-modul

[PATCH] D104350: [clang][AST] Make `getLocalOrImportedSubmoduleID` work with const `Module*`. NFC.

vsapsai created this revision. vsapsai added a reviewer: dexonsmith. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D104350 Files: clang/include/clang/Serialization/AS

[PATCH] D104351: [HeaderSearch] Use `isImport` only for imported headers and not for `#pragma once`.

vsapsai created this revision. vsapsai added reviewers: jansvoboda11, bruno. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. There is a separate field `isPragmaOnce` and when `isImport` combines both, it complicates HeaderFileInfo seri

[PATCH] D104351: [HeaderSearch] Use `isImport` only for imported headers and not for `#pragma once`.

vsapsai added inline comments. Comment at: clang/include/clang/Lex/HeaderSearch.h:444-447 void MarkFileIncludeOnce(const FileEntry *File) { HeaderFileInfo &FI = getFileInfo(File); -FI.isImport = true; FI.isPragmaOnce = true; This affects the resu

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai updated this revision to Diff 352326. vsapsai added a comment. Split out smaller parts as separate reviews. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://reviews.llvm.org/D104344 Files: clang/include/clang/Lex/HeaderS

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai updated this revision to Diff 352572. vsapsai added a comment. Test when a non-modular header *is* visible through a module import (mostly for completeness). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://reviews.llvm.or

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai updated this revision to Diff 352580. vsapsai added a comment. Add a test to verify `MarkFileIncludedFromModule` does `|= isImport` instead of `= isImport`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://reviews.llvm.org

[PATCH] D104351: [HeaderSearch] Use `isImport` only for imported headers and not for `#pragma once`.

vsapsai added a comment. In D104351#2824505 , @jansvoboda11 wrote: > This seems reasonable on first look. Can you add a test that demonstrates the > problem this patch solves? There is no externally observable change that can be tested, I'm only making

[PATCH] D104350: [clang][AST] Make `getLocalOrImportedSubmoduleID` work with const `Module*`. NFC.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG722c51473c7a: [clang][AST] Make `getLocalOrImportedSubmoduleID` work with const `Module*`. (authored by vsapsai). Repository: rG LLVM Github Mono

[PATCH] D104350: [clang][AST] Make `getLocalOrImportedSubmoduleID` work with const `Module*`. NFC.

vsapsai added a comment. Thanks for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104350/new/ https://reviews.llvm.org/D104350 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

vsapsai updated this revision to Diff 353081. vsapsai marked an inline comment as done. vsapsai added a comment. Rebase on top of "main" and address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/ https://reviews.llvm.org/

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

vsapsai added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:487 // Only calculate hash on first call of getODRHash per record. - ODRHash Hash; + class ODRHash Hash; Hash.AddCXXRecordDecl(getDefinition()); rsmith wrote: > I think this change is no

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

vsapsai updated this revision to Diff 363891. vsapsai added a comment. Handle nested anonymous structs and `IndirectFieldDecl`; more tests to cover unions and bitfields. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106994/new/ https://reviews.llv

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

vsapsai updated this revision to Diff 363894. vsapsai added a comment. Add missing changes back. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106994/new/ https://reviews.llvm.org/D106994 Files: clang/include/clang/Serialization/ASTReader.h cl

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

vsapsai added inline comments. Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3346-3347 + if (auto *RD = dyn_cast(DC)) +return RD->getDefinition(); + In D71734 we have ```lang=c++ if (auto *RD = dyn_cast(DC)) if (!RD->getASTContext().getLangO

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

vsapsai added inline comments. Comment at: clang/lib/AST/ASTDumper.cpp:199 P.setDeserialize(Deserialize); +P.doGetNodeDelegate().setDeserialize(Deserialize); P.Visit(this); It is possible it will complicate the implementation too much but what if `

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://reviews.llvm.org/D104344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

vsapsai added a comment. As long as JSON dumper still compiles, it looks good to me. Just have a few small nits. Comment at: clang/include/clang/AST/TextNodeDumper.h:47 + /// Indicates if we can deserialize declarations from the ExternalASTSource. + bool Deserialize = true;

[PATCH] D104351: [HeaderSearch] Use `isImport` only for imported headers and not for `#pragma once`.

vsapsai added a comment. Thanks for the review! I'll wait with committing this change until https://reviews.llvm.org/D104344 is ready to avoid bumping `VERSION_MAJOR` twice. While this change doesn't introduce any syntactical differences to serialized AST, it does introduce semantic differences

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

vsapsai added a comment. Made the first review pass and `return Failure` makes sense to me as recovery isn't the best idea at this point. Still want to check more thoroughly if the removed code for `SUBMODULE_UMBRELLA_HEADER` and `SUBMODULE_UMBRELLA_DIR` has any load-bearing side-effects. Have

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

vsapsai added a comment. Found another case that doesn't emit an error #if defined(FIRST) struct Indirect { int x; }; struct Direct { struct Indirect i; }; #elif defined(SECOND) struct Indirect { double a; }; struct Direct { struct Indirect i; }; #else stru

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://reviews.llvm.org/D104344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

vsapsai planned changes to this revision. vsapsai added a comment. Need to detect mismatches in nested structs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/ https://reviews.llvm.org/D71734 __

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

vsapsai added a comment. Why are you not just changing `return OutOfDate` to `return Failure` for SUBMODULE_UMBRELLA_HEADER and SUBMODULE_UMBRELLA_DIR? I have no strong opinion on this but it feels more in line with the rest of the changes. Also I've tried to do that and nothing seems to be bro

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

vsapsai accepted this revision. vsapsai added a comment. This revision is now accepted and ready to land. Looks good to me. The only nitpicky thing is I don't remember if the code style requires braces around multi-line case blocks or not. Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D107296: Treat inttypes.h as a built-in header

vsapsai added a comment. In general it is correct but there is no test case. I have a test case but it fails for other reasons. And I wasn't able to find time to address these "other reasons". I'll post my version of the test case soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

This revision was automatically updated to reflect the committed changes. Closed by commit rG32208555af26: [Modules] Do not remove failed modules after the control block phase (authored by bnbarham, committed by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

vsapsai added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:4268 // Read the AST block. if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities)) + return Failure; yaron.keren wrote: > Result is unused now. Thanks for

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

vsapsai added a comment. Tested clang with this change on internal code and there were no regressions. Also have done limited testing of runtime behavior of projects built with this clang - no errors encountered. So the testing so far hasn't found any issues. Repository: rG LLVM Github Monor

[PATCH] D104963: [ODR] Fix using uninitialized FunctionTypeBits.FastTypeQuals in FunctionNoProtoType.

vsapsai created this revision. vsapsai added reviewers: rsmith, riccibruno. Herald added a subscriber: ributzka. vsapsai requested review of this revision. Herald added a project: clang. FastTypeQuals are used only by FunctionProtoType and put in FunctionTypeBitfields in FunctionType to pack with

[PATCH] D104963: [ODR] Fix using uninitialized FunctionTypeBits.FastTypeQuals in FunctionNoProtoType.

vsapsai added a comment. Wasn't actually able to make the test fail because Memory Sanitizer isn't supported on macOS and on Linux I cannot even build clang with MSAN enabled. The test case is reduced from an actual error we've seen internally when clang complaints `Handler` has different defin

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai added a comment. Ping. It would be useful to know if this approach makes sense. If so, I can test it on a bigger codebase and see if there are any problems. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://reviews.llvm.org

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

vsapsai added a comment. This problem shouldn't happen based on module-level name hashing but for some reason it is happening. If I'm not mistaken, InMemoryModuleCache uses .pcm file name as the key. But that file name should contain a hash part that is based on the modulemap path. But framewor

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

vsapsai added inline comments. Comment at: clang/unittests/Serialization/ModuleCacheTest.cpp:125-126 + ASSERT_TRUE(Invocation2); + CompilerInstance Instance2(Instance.getPCHContainerOperations(), + &Instance.getModuleCache()); + Instance2.setDiagnos

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/ https://reviews.llvm.org/D71734 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-

[PATCH] D104344: [modules] Track how headers are included by different modules.

vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104344/new/ https://reviews.llvm.org/D104344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

vsapsai added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:1063 +<< ModuleName; +return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors; + } Can we get in infinite loop with `AllowPCMWithCompilerErrors = true`?

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

vsapsai added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:1063 +<< ModuleName; +return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors; + } bnbarham wrote: > bnbarham wrote: > > vsapsai wrote: > > > Can we get

[PATCH] D109632: [clang] de-duplicate methods from AST files

vsapsai added a comment. In D109632#3086044 , @rmaz wrote: > @vsapsai i'll abandon this diff then, thanks for your extensive feedback on > the approach. Is D110123 shippable > already, or are there some more corner cas

[PATCH] D110123: [Proof of concept] Serialize fewer transitive methods in `METHOD_POOL`.

vsapsai updated this revision to Diff 382477. vsapsai added a comment. Rebase and update the commit message. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110123/new/ https://reviews.llvm.org/D110123 Files: clang/lib/Serialization/ASTReader.cpp

[PATCH] D110123: [clang][objc] Speed up populating the global method pool from modules.

vsapsai added a comment. /cc @v.g.vassilev as he mentioned an issue with late-parsed templates can be similar to this one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110123/new/ https://reviews.llvm.org/D110123

[PATCH] D110123: [clang][objc] Speed up populating the global method pool from modules.

vsapsai added a comment. Lots of relevant discussion can be found at https://reviews.llvm.org/D109632 I'll add the link to it to the summary later, for now the summary is the same as commit message. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D11

[PATCH] D111860: [modules] Update visibility for merged ObjCProtocolDecl definitions.

vsapsai updated this revision to Diff 383201. vsapsai added a comment. Rebase and trigger pre-merge checks again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111860/new/ https://reviews.llvm.org/D111860 Files: clang/lib/Serialization/ASTReader

[PATCH] D112915: WIP: [clang][modules] Granular tracking of includes

vsapsai added a comment. I'm not going to cover the entire change, some parts I need to consider more carefully. There can be other reasons to keep `IncludeMap` out of `SubmoduleState` but I'm not sure the local submodule visibility is the right reason. I might be reading the code incorrectly

[PATCH] D110123: [clang][objc] Speed up populating the global method pool from modules.

vsapsai updated this revision to Diff 384606. vsapsai added a comment. Rebase. Hopefully MLIR build is fixed and I'll be able to see relevant pre-merge check results. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110123/new/ https://reviews.llvm.o

[PATCH] D110123: [clang][objc] Speed up populating the global method pool from modules.

This revision was automatically updated to reflect the committed changes. Closed by commit rG0a35cc40b881: [clang][objc] Speed up populating the global method pool from modules. (authored by vsapsai). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110

[PATCH] D110123: [clang][objc] Speed up populating the global method pool from modules.

vsapsai added a comment. Thanks for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110123/new/ https://reviews.llvm.org/D110123 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://

[PATCH] D112915: WIP: [clang][modules] Granular tracking of includes

vsapsai added a comment. In D112915#3106472 , @jansvoboda11 wrote: > That's interesting. I think `HeaderFileInfo::isImport` should definitely be > tracked in the preprocessor, not in `HeaderFileInfo`. The fact that the > header was `#import`ed is not a

[PATCH] D111860: [modules] Update visibility for merged ObjCProtocolDecl definitions.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7ad693a322c1: [modules] Update visibility for merged ObjCProtocolDecl definitions. (authored by vsapsai). Repository: rG LLVM Github Monorepo CHA

<    2   3   4   5   6   7   8   9   10   11   >