[PATCH] D33393: [PATCH] Libcxxabi Demangler PR32890

dexonsmith added a reviewer: erik.pilkington. dexonsmith added inline comments. Comment at: src/cxa_demangle.cpp:3078-3079 ++t1; -db.names.back().first.insert(db.names.back().first.begin()+7, t0, t1); +if (db.names.back().firs

[PATCH] D117348: [Preprocessor] Reduce the memory overhead of `#define` directives

dexonsmith added inline comments. Comment at: clang/lib/Lex/MacroInfo.cpp:33 + +// MacroInfo is expected to take 40 bytes on platforms with an 8 byte pointer. +template class MacroInfoSizeChecker { aaron.ballman wrote: > Should we do this dance for 32-bit system

[PATCH] D114966: [clang][deps] Split stat and file content caches

dexonsmith accepted this revision. dexonsmith 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/D114966/new/ https://reviews.llvm.org/D114966 __

[PATCH] D117348: [Preprocessor] Reduce the memory overhead of `#define` directives

dexonsmith added inline comments. Comment at: clang/lib/Lex/MacroInfo.cpp:33 + +// MacroInfo is expected to take 40 bytes on platforms with an 8 byte pointer. +template class MacroInfoSizeChecker { aaron.ballman wrote: > dexonsmith wrote: > > aaron.ballman wrote

[PATCH] D117730: [DNM][VFS] Do not overwrite the path when nesting RedirectingFileSystems

dexonsmith added a comment. @keith, any thoughts on this? IIRC, you'd found a few other bugs while you were in there which you'd left for later; not sure if this is related to them or not. Comment at: clang/test/VFS/directory.c:2 // RUN: rm -rf %t -// RUN: mkdir -p %t/Underly

[PATCH] D117730: [DNM][VFS] Do not overwrite the path when nesting RedirectingFileSystems

dexonsmith added inline comments. Comment at: clang/test/VFS/directory.c:2 // RUN: rm -rf %t -// RUN: mkdir -p %t/Underlying -// RUN: mkdir -p %t/Overlay -// RUN: mkdir -p %t/Middle -// RUN: echo '// B.h in Underlying' > %t/Underlying/B.h -// RUN: echo '#ifdef NESTED' >> %t/Unde

[PATCH] D117746: [CMake] Pass CMAKE_C/CXX_COMPILER_LAUNCHER down to cross-compile and runtime build

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Sure, seems reasonable to me. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117746/new/ https://reviews.llvm.org/D117746

[PATCH] D117937: [VFS] Add a "redirecting-with" field to overlays

dexonsmith added a comment. Haven't had a chance to look at the code (maybe @keith/@vsapsai/others will have time before I do) but at a high-level this seems pretty sound / SGTM. The scenario you describe reminds me of the testcase in https://reviews.llvm.org/D117730 ; can you explain if/how th

[PATCH] D118109: [clang] Replace `std::vector` use in Syntax

dexonsmith accepted this revision. dexonsmith added a comment. LGTM too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118109/new/ https://reviews.llvm.org/D118109 ___ cfe-commits mailing list cfe-commit

[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration

dexonsmith added a comment. 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 a

[PATCH] D118311: [Clang][ModuleMap] Add conditional parsing via requires block declaration

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:722 +def err_mmap_expected_lbrace_requires : Error< + "expected '{' to start rquires block">; def err_mmap_expected_rbrace : Error<"expected '}'">; Bigcheese wrote: >

[PATCH] D117348: [Preprocessor] Reduce the memory overhead of `#define` directives

dexonsmith added a comment. LGTM once @aaron.ballman is happy. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117348/new/ https://reviews.llvm.org/D117348 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/m

[PATCH] D120474: [clang][deps] Remove '-fmodules-cache-path=' arguments

dexonsmith added inline comments. Herald added a project: All. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:36 + llvm::erase_if(Args, [](const std::string &Arg) { +return Arg.find("-fmodules-cache-path=") == 0; + }); Is that t

[PATCH] D120465: [clang][deps] Generate necessary "-fmodule-map-file=" arguments, disable implicit module maps

dexonsmith added inline comments. Herald added a project: All. Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:112 void dependencies::detail::collectPCMAndModuleMapPaths( llvm::ArrayRef Modules, Should this be renamed? ==

[PATCH] D120463: [clang][modules] NFC: Simplify and clarify test

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Herald added a project: All. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120463/new/ https://reviews.llvm.org/D120463 _

[PATCH] D120464: [clang][modules] Report module maps affecting `no_undeclared_includes` modules

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Herald added a project: All. LGTM, with one test suggestion. Comment at: clang/test/Modules/add-remove-irrelevant-module-map.m:36-38 +#if __has_include("d.h") // This

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

dexonsmith added a comment. Herald added a project: All. In D112349#3330918 , @pirama wrote: > Unrelated to missing resolver definition, this change doesn't accommodate > resolvers that take parameters. (Curiously, this verification only fails > with T

[PATCH] D101000: Coverage: Document how to collect a profile without a filesystem

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. dexonsmith marked an inline comment as done. Closed by commit rGd4ee603c8f21: Coverage: Document how to collect a profile without a filesystem (authored by dexonsmith).

[PATCH] D101051: [clang][deps] Only generate absolute paths when asked to

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with one nit to pick. Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:392 std::vector ModuleDeps; -std::vector AdditonalCommandLine; +std

[PATCH] D101192: Add support for #elifdef and #elifndef

dexonsmith added a comment. I reviewed the source minimizer sections, and the code looks correct. It'd probably be good to have unit tests in clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp for the incremental changes. - Do the new constructs stick around in the minimized sourc

[PATCH] D100872: Use OpenFlags instead of boolean to set a file as text/binary

dexonsmith added a subscriber: sammccall. dexonsmith added inline comments. Comment at: clang/include/clang/Frontend/CompilerInstance.h:738-740 + createOutputFileImpl(StringRef OutputPath, llvm::sys::fs::OpenFlags Flags, bool RemoveFileOnSignal, bool UseT

[PATCH] D101506: Support: Remove F_{None,Text,Append} compatibility synonyms, NFC

dexonsmith created this revision. dexonsmith added a reviewer: zturner. Herald added subscribers: dcaballe, cota, teijeong, rdzhabarov, tatianashp, bmahjour, msifontes, jurahul, Kayjukh, grosul1, jvesely, Joonsoo, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, antiagainst, shauheen, rriddle

[PATCH] D101506: Support: Remove F_{None,Text,Append} compatibility synonyms, NFC

dexonsmith added a comment. In D101506#2724746 , @MaskRay wrote: > Perhaps split the migration part from the `OpenFlags` removal part. I thought > about deleting the aliases too but refrained because I noticed our internal > code base needs migration. >

[PATCH] D101650: Support: Stop using F_{None,Text,Append} compatibility synonyms, NFC

dexonsmith created this revision. dexonsmith added a reviewer: MaskRay. Herald added subscribers: dcaballe, cota, teijeong, rdzhabarov, tatianashp, bmahjour, msifontes, jurahul, Kayjukh, grosul1, jvesely, Joonsoo, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, antiagainst, shauheen, rriddle

[PATCH] D101506: Support: Remove F_{None,Text,Append} compatibility synonyms, NFC

dexonsmith updated this revision to Diff 341966. dexonsmith edited the summary of this revision. dexonsmith added a comment. Rebased on top of https://reviews.llvm.org/D101506 -- that updates all in-tree users, and now this is just removing the API. CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D101650: Support: Stop using F_{None,Text,Append} compatibility synonyms, NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG518d955f9dd2: Support: Stop using F_{None,Text,Append} compatibility synonyms, NFC (authored by dexonsmith). Repository: rG LLVM Github Monorepo

[PATCH] D101506: Support: Remove F_{None,Text,Append} compatibility synonyms, NFC

dexonsmith added a comment. In D101506#2729839 , @MaskRay wrote: > Perhaps give us and other projects one week or longer ... Sure thing; I'll try committing next week some time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101506/new/ https:/

[PATCH] D101667: Modules: Remove ModuleLoader::OtherUncachedFailure, NFC

dexonsmith created this revision. dexonsmith added reviewers: jansvoboda11, Bigcheese, rsmith. dexonsmith requested review of this revision. Herald added a project: clang. 5cca622310c10fdf6f921b6cce26f91d9f14c762 (https://revi

[PATCH] D101670: Modules: Rename ModuleBuildFailed => DisableGeneratingGlobalModuleIndex, NFC

dexonsmith created this revision. dexonsmith added reviewers: jansvoboda11, Bigcheese, rsmith. Herald added a subscriber: arphaman. dexonsmith requested review of this revision. Herald added a project: clang. Rename CompilerInstance's ModuleBuildFailed field to DisableGeneratingGlobalModuleIndex,

[PATCH] D101671: Modules: Remove an extra early return, NFC

dexonsmith created this revision. dexonsmith added reviewers: jansvoboda11, Bigcheese. dexonsmith requested review of this revision. Herald added a project: clang. Remove an early return from an `else` block that's immediately followed by an equivalent early return after the `else` block. Reposi

[PATCH] D101672: Modules: Simplify how DisableGeneratingGlobalModuleIndex is set, likely NFC

dexonsmith created this revision. dexonsmith added reviewers: jansvoboda11, Bigcheese, rsmith. Herald added a subscriber: arphaman. dexonsmith requested review of this revision. Herald added a project: clang. DisableGeneratingGlobalModuleIndex was being set by CompilerInstance::findOrCompileModule

[PATCH] D101672: Modules: Simplify how DisableGeneratingGlobalModuleIndex is set, likely NFC

dexonsmith added a comment. Just fixed a typo in the description, where the second paragraph should start: > The extra cases where this is set are all some version of a fatal error, > and the only client of the field, shouldBuildGlobalModuleIndex, seems > to be unreachable in that case. (and now

[PATCH] D100934: [clang][modules] Build inferred modules

dexonsmith added a comment. Given that there are four different things being done in this commit, it sounds naively like it should be four separate commits. If the logic is too intertwined to land each of the four pieces separately (certainly possible), can you quickly explain why, to motivate

[PATCH] D101671: Modules: Remove an extra early return, NFC

This revision was automatically updated to reflect the committed changes. Closed by commit rG64a390c1bc75: Modules: Remove an extra early return, NFC (authored by dexonsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101671/new/ https://reviews

[PATCH] D101667: Modules: Remove ModuleLoader::OtherUncachedFailure, NFC

dexonsmith added a comment. Just marked this as a "parent" of https://reviews.llvm.org/D101670 and https://reviews.llvm.org/D101672 since the patches conflict, but if for some reason this one is controversial I *can* rebase them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D101758: [clang][modules] Add -cc1 option to backup PCM files

dexonsmith added a comment. A few high-level comments: - Something like this *does* seem really useful for debugging problems with implicit module builds using a fuzzy context hash. But I feel like the use case is carved out at the wrong place; I feel like we either want this to be: - more re

[PATCH] D101775: Fix for Bug 50033 - -fno-temp-file is not respected when creating a pch in clang 12

dexonsmith added a comment. Yep, this fixes a regression (https://bugs.llvm.org/show_bug.cgi?id=50033) caused by ad7aaa475e5e632242b07380ec47d2f35d077209 , where I somehow missed that modules and PCHs had different actual beha

[PATCH] D100934: [clang][modules] Build inferred modules

dexonsmith added a comment. In D100934#2735955 , @jansvoboda11 wrote: > In D100934#2733857 , @dexonsmith > wrote: > >> Given that there are four different things being done in this commit, it >> sounds naively

[PATCH] D100934: [clang][modules] Build inferred modules

dexonsmith added inline comments. Comment at: clang/test/ClangScanDeps/modules-inferred-explicit-build.m:13-17 +// RUN: %clang @%t.inferred.rsp -pedantic -Werror +// RUN: %clang @%t.system.rsp -pedantic -Werror +// RUN: %clang -x objective-c -fsyntax-only %t.dir/modules_cdb_input

[PATCH] D100934: [clang][modules] Build inferred modules

dexonsmith added inline comments. Comment at: clang/test/ClangScanDeps/modules-inferred-explicit-build.m:13-17 +// RUN: %clang @%t.inferred.rsp -pedantic -Werror +// RUN: %clang @%t.system.rsp -pedantic -Werror +// RUN: %clang -x objective-c -fsyntax-only %t.dir/modules_cdb_input

[PATCH] D101667: Modules: Remove ModuleLoader::OtherUncachedFailure, NFC

dexonsmith added a comment. Ping! Happy to weaken the commit message to "likely NFC" in case there's some way that we get back here after a fatal error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101667/new/ https://reviews.llvm.org/D101667 __

[PATCH] D102261: Introduce SYCL 2020 mode

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:130 enum SYCLMajorVersion { SYCL_None, aaron.ballman wrote: > tschuett wrote: > > Do you want to change it to a scoped enum or will this cause major issues? > > Clan

[PATCH] D116960: [ADT] Add an in-place version of toHex()

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Mostly LGTM, just a couple of comments on the drive-by changes to `hexdigit()` (I might split that (and the new call in toHex()) into a separate commit, but if you keep them here pleas

[PATCH] D116659: [llvm][clang][vfs] NFC: Extract directory iteration boilerplate into macro

dexonsmith added a comment. I think th eAPI is designed to ensure users don't forget to check the error code. What about a new API/iterator type that wraps the existing one and has a "normal" `operator++`? - Take an error as an out-parameter on construction - Make it an `llvm::Error` which gua

[PATCH] D115379: ASTMatchers: Avoid using SmallVector::set_size()

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf0b2a1a629da: ASTMatchers: Avoid using SmallVector::set_size() (authored by dexonsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D115386: AST: Avoid using SmallVector::set_size() in UnresolvedSet

This revision was automatically updated to reflect the committed changes. Closed by commit rG3110060bc721: AST: Avoid using SmallVector::set_size() in UnresolvedSet (authored by dexonsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115386/new/

[PATCH] D114966: [clang][deps] Split stat and file content caches

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. Okay, I think this is the last round. Everything looks correct, except a few out-of-date comments. Two things, one small, one bigger (but not too big I think). Smaller one i

[PATCH] D117119: [clangd][clang-tidy] Remove uses of `std::vector`

dexonsmith accepted this revision. dexonsmith 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/D117119/new/ https://reviews.llvm.org/D117119 __

[PATCH] D116659: [llvm][clang][vfs] NFC: Simplify directory iteration

dexonsmith added a subscriber: benlangmuir. dexonsmith added a comment. In D116659#3237061 , @jansvoboda11 wrote: > I agree that introducing new iterator and implementing `iterator_range<...> > FileSystem::dir_range()` is better solution than a macro. >

[PATCH] D116659: [llvm][clang][vfs] NFC: Simplify directory iteration

dexonsmith added a comment. In D116659#3241727 , @benlangmuir wrote: >> I don't know, I'm a bit skeptical we want to make it so easy to ignore >> errors so easily. I'd rather require clients to explicitly ignore the error. > > That's my gut feeling too.

[PATCH] D116174: [clang] support relative roots to vfs overlays

dexonsmith added subscribers: vsapsai, benlangmuir. dexonsmith added a comment. In D116174#3206895 , @keith wrote: > This LGTM but I'm interested to hear from folks with more knowledge about the > VFS' desired behavior to comment on if this will have any

[PATCH] D115043: [clang][deps] Use MemoryBuffer in minimizing FS

dexonsmith added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:64-70 MinimizedFileContents.push_back('\0'); - Result.Contents = std::move(MinimizedFileContents); - // Now make the null terminator implicit again, so that C

[PATCH] D114966: [clang][deps] Split filesystem caches

dexonsmith added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:253-255 + if (SharedOriginalContents.WasInserted) +SharedStat->Stat = +readFile(Filename, getUnderlyingFS(), SharedOriginalContents.Value); --

[PATCH] D115254: Revert "Revert "Use VersionTuple for parsing versions in Triple, fixing issues that caused the original change to be reverted. This makes it possible to distinguish between "16" and "

dexonsmith added a comment. In D115254#3176564 , @jamesfarrell wrote: > Only change from previous attempt is to call rtrim() on the output of the > commands in the unit tests, since the new version parsing code fails if there > are leftover characters

[PATCH] D115331: [llvm] Add null-termination capability to SmallVectorMemoryBuffer

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, except I'd prefer the `#include`s be changed separately since that cleanup seems unrelated to this change. Comment at: lldb/unittests/Expression/CppModuleConfi

[PATCH] D115043: [clang][deps] Use MemoryBuffer in minimizing FS

dexonsmith accepted this revision. dexonsmith 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/D115043/new/ https://reviews.llvm.org/D115043 __

[PATCH] D115378: OpenMP: Avoid using SmallVector::set_size()

dexonsmith created this revision. dexonsmith added a reviewer: Meinersbur. Herald added subscribers: guansong, hiraditya, yaxunl. dexonsmith requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. Herald added projects: clang, LLVM. Update `OpenM

[PATCH] D115379: ASTMatchers: Avoid using SmallVector::set_size()

dexonsmith created this revision. dexonsmith added reviewers: njames93, aaron.ballman. dexonsmith requested review of this revision. Herald added a project: clang. Update `variadicMatcherDescriptor` to assert on reserved capacity and to call `emplace_back()` instead of calling `set_size()` and con

[PATCH] D115386: AST: Avoid using SmallVector::set_size() in UnresolvedSet

dexonsmith created this revision. dexonsmith added a reviewer: dblaikie. dexonsmith requested review of this revision. Herald added a project: clang. Update UnresolvedSet to use (and expose) `SmallVector::truncate()` instead of `SmallVector::set_size()`. The latter is going to made private in a fu

[PATCH] D115346: [clang][deps] Squash caches for original and minimized files

dexonsmith added a comment. This looks really nice. One problem is that it's subtle to confirm whether it's thread-safe. The local cache access parts of CachedFileSystemEntry lock-free, but the CachedFileSystemEntry might get changed later (i.e., filled in with minimized content). Are accessed

[PATCH] D115378: OpenMP: Avoid using SmallVector::set_size()

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcfd1d49dc0cc: OpenMP: Avoid using SmallVector::set_size() (authored by dexonsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D115332: [clang][deps] Use lock_guard instead of unique_lock

dexonsmith accepted this revision. dexonsmith 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/D115332/new/ https://reviews.llvm.org/D115332 ___

[PATCH] D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently

dexonsmith added a comment. I don't feel strongly, but IMO the code might be a bit harder to read/maintain with the explicit flush. I worry that it'd be easy to move the `flush()` away from the `return`. Not sure I'm right; could just be familiarity with `str()`. Have you considered other optio

[PATCH] D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently

dexonsmith added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h:36 +OS.flush(); +return Str; } logan-5 wrote: > Quuxplusone wrote: > > FWIW, it appears to me that in most (all?) of these cases, what's really >

[PATCH] D115386: AST: Avoid using SmallVector::set_size() in UnresolvedSet

dexonsmith updated this revision to Diff 392977. dexonsmith added reviewers: Bigcheese, jansvoboda11, vsapsai. dexonsmith added a comment. Adding a couple of other possible reviewers. [no change, just retriggering bots after spurious failure] CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D115386: AST: Avoid using SmallVector::set_size() in UnresolvedSet

dexonsmith added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:623 - Decls.set_size(N); + Decls.truncate(N); Two things to confirm here. First is that the destructors are trivial. From clang/include/clang/AST/DeclAccessPair.h: ``` lang=c++ cl

[PATCH] D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently

dexonsmith added a comment. In D115374#3181491 , @logan-5 wrote: > In D115374#3181383 , @dexonsmith > wrote: > >> I don't feel strongly, but IMO the code might be a bit harder to >> read/maintain with the explic

[PATCH] D113254: [clang] Fix a misadjusted path style comparison in a unittest

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM; sorry for missing this previously! (I'd have been happy for you to commit this without review, BTW, since it seems a bit obvious, but no excuse of course for me being slow...)

[PATCH] D114505: [clang][unittests] Fix a clang unittest linking issue

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Sorry for missing this before. Looks to me like the debian build failure above is unrelated. If you need someone to commit for you, please include the info for GIT_AUTHOR_NAME a

[PATCH] D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently

dexonsmith added a comment. In D115374#3181670 , @logan-5 wrote: > To be clear, it sounds like we should //either// add `.take()` for moving the > string out of `raw_string_ostream`'s string reference, //or// make > `raw_string_ostream` not need to be f

[PATCH] D115415: [clang][macho] add clang frontend support for emitting macho files with two build version load commands

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. This LGTM, but I came across it pretty quickly; might be worth leaving for a couple of days in case someone else has comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D11

[PATCH] D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently

dexonsmith added a comment. Returning from https://reviews.llvm.org/D115421, it looks like that `raw_string_ostream` has unbuffered by default since 65b13610a5226b84889b923bae884ba395ad084d . Seems like all the new `flush()`s

[PATCH] D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. In D115374#3183776 , @logan-5 wrote: > Removed `.flush()`es. Seems like this might be able to land without > https://reviews.llvm.org/D115421?

[PATCH] D115346: [clang][deps] Squash caches for original and minimized files

dexonsmith added inline comments. Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:106-108 + std::unique_ptr OriginalContents; + std::unique_ptr MinimizedContents; PreprocessorSkippedRangeMapping PPSkippedRangeMapping; --

[PATCH] D115355: Fix build failure with GCC 11 in C++20 mode

dexonsmith accepted this revision. dexonsmith added a comment. LGTM too. Comment at: clang/lib/AST/ASTImporter.cpp:8400-8403 + AttrArgImporter(const AttrArgImporter &) = delete; + AttrArgImporter(AttrArgImporter &&) = default; AttrArgImporter &operator=(const AttrArgImport

[PATCH] D109981: [Diagnostics] Don't drop a statically set NoWarningAsError flag during option processing

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/test/Lexer/pragma-message.c:14 #define STRING(x) STRING2(x) -#pragma message(":O I'm a message! " STRING(__LINE__)) // expected-warning {{:O

[PATCH] D115346: [clang][deps] Squash caches for original and minimized files

dexonsmith added inline comments. Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:106-108 + std::unique_ptr OriginalContents; + std::unique_ptr MinimizedContents; PreprocessorSkippedRangeMapping PPSkippedRangeMapping; --

[PATCH] D114505: [clang][unittests] Fix a clang unittest linking issue

dexonsmith added a comment. Lost track of this again :/. Was just about to commit this for you (finally) but it looks like https://reviews.llvm.org/D115580 has already fixed it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114505/new/ https://reviews.llvm.org/D114505

[PATCH] D114966: [clang][deps] Split stat and file content caches

dexonsmith added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:25-31 llvm::ErrorOr Stat = (*MaybeFile)->status(); if (!Stat) return Stat.getError(); llvm::vfs::File &F = **MaybeFile; llvm::ErrorOr> MaybeBuff

[PATCH] D115346: [clang][deps] Squash caches for original and minimized files

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM after you fix one more race (and add good code comments). Reads of `PPSkippedRangeMappings` don't happen until after `MinimizedContentsAccess` is checked, but the writes need to h

[PATCH] D114966: [clang][deps] Split stat and file content caches

dexonsmith added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:200-210 + // FIXME: This read can fail. + // In that case, we should probably update `CacheEntry::MaybeStat`. + // However, that is conc

[PATCH] D114966: [clang][deps] Split stat and file content caches

dexonsmith added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:110-111 + +CachedFileContents &DependencyScanningFilesystemSharedCache::getFileContents( +StringRef Filename, llvm::sys::fs::UniqueID UID) { + CacheShard &Sha

[PATCH] D115935: [clang][deps] NFC: Simplify handling of cached FS errors

dexonsmith accepted this revision. dexonsmith 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/D115935/new/ https://reviews.llvm.org/D115935 __

[PATCH] D114966: [clang][deps] Split stat and file content caches

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. I'm liking the new direction here; requesting changes since it looks like the filename is being used to pick the shard for UIDMap, which will lead to multiple opinions of wha

[PATCH] D114971: [clang][deps] Handle symlinks in minimizing FS

dexonsmith accepted this revision. dexonsmith 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/D114971/new/ https://reviews.llvm.org/D114971 __

[PATCH] D116110: Introduce the AttributeMask class

dexonsmith added a comment. In D116110#3205416 , @nikic wrote: > This looks good to me, but I'd recommend waiting a bit before landing in case > there's further input, as this is a non-trivial API change. (Maybe valuable to start a quick thread on llvm-

[PATCH] D97850: Fix PCM read from ModuleCache for ext4 filesystem

dexonsmith added a comment. In D97850#2629464 , @ilyakuteev wrote: > If a fix will be in ModuleManager and only for ModuleCache the problem with > symlinks and path will not affect it as ModuleCache is managed by clang and > we can rely on it. > I agree

[PATCH] D98943: [clang][deps] NFC: Extract ModuleID struct

dexonsmith accepted this revision. dexonsmith 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/D98943/new/ https://reviews.llvm.org/D98943

[PATCH] D99568: [clang][invocation] Fix copy constructor, add copy assignment to CompilerInvocation

dexonsmith added a comment. I'm not sure a deep copy is entirely sound, due to odd ownership rules in "remapped buffers" (I forget the subclass that includes those). A few months ago I got most of the way to deleting those (lifting the logic up into the clang tooling that needed it)... maybe we

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

dexonsmith added a comment. In D97462#2666485 , @jansvoboda11 wrote: > Thanks for reporting that. D99606 fixes one > aspect of `-plugin-arg`, but it seems the order of generation is > non-deterministic (most likely rela

[PATCH] D99879: [clang][cli] Ensure plugin args are generated in deterministic order

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Based on git-blame, this was added in 6c78974b298d619ec11e243fb6fdea0b16f87a66 (https://reviews.llvm.org/D17959) and the choice of unordered vs. ordered looks incidental (not mot

[PATCH] D99568: [clang][invocation] Fix copy constructor, add copy assignment to CompilerInvocation

dexonsmith added a comment. In D99568#2682815 , @jansvoboda11 wrote: > Ping. Thanks for the ping; I'd completely missed your follow-up. Just fixing AnalyzerOptions for now sounds great. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D99568: [clang][invocation] Fix copy constructor of CompilerInvocation

dexonsmith accepted this revision. dexonsmith 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/D99568/new/ https://reviews.llvm.org/D99568

[PATCH] D100428: [clang][FileManager] Support empty file name in getVirtualFileRef for serialized diagnostics

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Can you add a comment to the code explaining why the filename could be empty? With that, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D100531: [clang][deps] Simplify function discovering .pcm and .modulemap files

dexonsmith accepted this revision. dexonsmith 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/D100531/new/ https://reviews.llvm.org/D100531 __

[PATCH] D100534: [clang][deps] Generate the full command-line for modules

dexonsmith added inline comments. Comment at: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:78-80 + /// The compiler invocation associated with the translation unit that imports + /// this module. + CompilerInvocation Invocation; Looks l

[PATCH] D100536: [clang][deps] NFC: Remove unused FullDependencies member

dexonsmith accepted this revision. dexonsmith 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/D100536/new/ https://reviews.llvm.org/D100536 __

[PATCH] D100644: [clang][cli] NFC: Use Diags to report parsing success/failure

dexonsmith accepted this revision. dexonsmith 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/D100644/new/ https://reviews.llvm.org/D100644 __

[PATCH] D100534: [clang][deps] Generate the full command-line for modules

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, just a couple of other comments inline. Comment at: clang/include/clang/Frontend/CompilerInstance.h:230 + + std::shared_ptr getInvocationPtr() { assert(In

[PATCH] D100671: [ADT] Factor out in_place_t and expose in Optional ctor

dexonsmith added a comment. In D100671#2696123 , @scott.linder wrote: > In D100671#2695923 , @dblaikie > wrote: > >> This usage doesn't seem to quite match the standard - which provides an >> existing instance

<    8   9   10   11   12   13   14   15   >