[PATCH] D119825: [clang][lex] Introduce `SearchDirIndex` to usage tracking code

ahoppen added a comment. Definitely a lot nicer  Comment at: clang/include/clang/Lex/HeaderSearch.h:185 + /// Get search directory stored at the index. + DirectoryLookup (HeaderSearch ) const; +}; I would have expected these to be methods on `HeaderSearch`.

[PATCH] D119722: [clang][lex] Use `SearchDirIterator` types in for loops

ahoppen accepted this revision. ahoppen added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Lex/HeaderSearch.cpp:1450 Optional HeaderSearch::searchDirIdx(const DirectoryLookup ) const { - for (unsigned I = 0; I < SearchDirs.size();

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

ahoppen accepted this revision. ahoppen added a comment. This revision is now accepted and ready to land. Assuming that this is indeed never used, removing dead code always sounds good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D119722: [clang][lex] Use `SearchDirIterator` types in for loops

ahoppen added a comment. Nice. I think this reads a lot better than the index-based implementation. Comment at: clang/lib/Lex/HeaderSearch.cpp:1450 Optional HeaderSearch::searchDirIdx(const DirectoryLookup ) const { - for (unsigned I = 0; I < SearchDirs.size(); ++I) -if

[PATCH] D119721: [clang][lex] Use `ConstSearchDirIterator` in lookup cache

ahoppen added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:710 + CacheLookup.HitIt = HitIt; + noteLookupUsage(&*HitIt - &*search_dir_begin(), Loc); } I haven’t looked into this in total details but `&*` looks a little awkward to me.

[PATCH] D119716: [clang][lex] NFC: De-duplicate some #include_next logic

ahoppen added inline comments. Comment at: clang/include/clang/Lex/Preprocessor.h:2207 + const FileEntry *) const; + /// Install the standard preprocessor pragmas: I there a reason why this uses an out parameter instead of a

[PATCH] D117312: [clang][lex] NFC: Simplify calls to `LookupFile`

ahoppen accepted this revision. ahoppen added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117312/new/ https://reviews.llvm.org/D117312

[PATCH] D117309: [clang] NFC: Remove unused `DirectoryLookup`

ahoppen accepted this revision. ahoppen added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117309/new/ https://reviews.llvm.org/D117309

[PATCH] D116750: [clang][lex] Keep references to `DirectoryLookup` objects up-to-date

ahoppen added inline comments. Comment at: clang/include/clang/Lex/HeaderSearch.h:179 /// directory is suppressed. - std::vector SearchDirs; - /// Whether the DirectoryLookup at the corresponding index in SearchDirs has - /// been successfully used to lookup a file. -

[PATCH] D116750: [clang][lex] Keep references to `DirectoryLookup` objects up-to-date

ahoppen added a comment. I like this a lot better. Some comments inline. Comment at: clang/include/clang/Lex/HeaderSearch.h:179 /// directory is suppressed. - std::vector SearchDirs; - /// Whether the DirectoryLookup at the corresponding index in SearchDirs has - ///

[PATCH] D116751: [clang][lex] NFC: Extract module creation into function

ahoppen accepted this revision. ahoppen added a comment. This revision is now accepted and ready to land. Ah, in that case it makes sense as-is. I just assumed they lived in the same library. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D116750: [clang][lex] Keep search directory indices up-to-date

ahoppen added a comment. I would prefer to use `std::shared_ptr` instead of `unsigned`. IIUC we already basically have a level of indirection because if we want to get the `DirectoryLookup`, we need to find it by its index in `SearchDirs`. And updating the indices just seems like a hack to me

[PATCH] D116751: [clang][lex] NFC: Extract module creation into function

ahoppen added a comment. I just want to make sure that we don’t accidentally introduce a new instantiation of a `Module` that doesn’t go through `makeModule` in the future. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116751/new/

[PATCH] D116750: [clang][lex] Keep search directory indices up-to-date

ahoppen added a comment. Adjusting the indices seem pretty fragile to me. Any reason why you wanted to stick with indices as keys instead of switching to something else like I suggested here ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D116751: [clang][lex] NFC: Extract module creation into function

ahoppen added a comment. I suppose the idea is that all `Module` creations should go through `makeModule`, right? In that case I think we should either - make the `Module` constructor private and `ModuleMap` a friend of `Module` - or at least add a doc comment to the `Module` constructor that

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

ahoppen added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:95 + if (HS.CurrentSearchPathIdx != ~0U) +HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx; +} These indices will be out of date if the search paths are changed via

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

ahoppen added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:94 + // Module map parsing initiated by header search. + if (HS.CurrentSearchPathIdx != ~0U) +HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx; jansvoboda11 wrote: >

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

ahoppen added a comment. LGTM overall. I’ve got a few questions/remarks inline. Comment at: clang/lib/Lex/HeaderSearch.cpp:94 + // Module map parsing initiated by header search. + if (HS.CurrentSearchPathIdx != ~0U) +HS.ModuleToSearchDirIdx[M] =

[PATCH] D102001: [Index] Ignore nullptr decls for indexing

ahoppen created this revision. Herald added a subscriber: arphaman. ahoppen requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We can end up with a call to `indexTopLevelDecl(D)` with `D == nullptr` in non-assert builds e.g. when indexing a

[PATCH] D96049: [Timer] On macOS count number of executed instructions

ahoppen added inline comments. Comment at: llvm/CMakeLists.txt:656 + list(APPEND CMAKE_REQUIRED_LIBRARIES proc) +endif() + thakis wrote: > also, any reason to not do this in `llvm/cmake/config-ix.cmake` where all the > other checks like this are? No, I just

[PATCH] D96049: [Timer] On macOS count number of executed instructions

ahoppen updated this revision to Diff 321467. ahoppen added a comment. Fixed an issue caused by me using an old revions as the commit's base. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96049/new/ https://reviews.llvm.org/D96049 Files:

[PATCH] D96049: [Timer] On macOS count number of executed instructions

ahoppen created this revision. Herald added subscribers: dexonsmith, hiraditya, mgorny. ahoppen requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. In addition to wall time etc. this should allow us to get less noisy values