[Lldb-commits] [PATCH] D52375: [WIP] Support multiple compile units per OSO entry in SymbolFileDWARFDebugMap

dexonsmith added inline comments. Comment at: packages/Python/lldbsuite/test/macosx/debug_map/Makefile:22 +# Everything goes as .o files directly to the linker +C_SOURCES := + aprantl wrote: > same here, just remove it I haven't read the full patch to see if thi

[Lldb-commits] [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

[Lldb-commits] [PATCH] D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. I think you didn't upload the full patch, so I think the bots will fail, but once the bots are happy this LGTM (besides one nit inline). Comment at: llvm/include/llv

[Lldb-commits] [PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

dexonsmith added a comment. > Includes two test fixes (since chained mappings are no longer allowed) > and adds a new test for multiple overlays. Are we sure no one needs to support chained mappings? Has there been a ~~clang-dev~~ discourse discussion about it already? Just concerned that some

[Lldb-commits] [PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

dexonsmith added a comment. In D121425#3384188 , @bnbarham wrote: > There's two failing tests with this change: > > - VFSFromYAMLTest.ReturnsExternalPathVFSHit > - VFSFromYAMLTest.ReturnsInternalPathVFSHit > > Apparently we allow relative paths in externa

[Lldb-commits] [PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

dexonsmith added a comment. In D121425#3384479 , @bnbarham wrote: > In D121425#3384188 , @bnbarham > wrote: > >> There's two failing tests with this change: >> >> - VFSFromYAMLTest.ReturnsExternalPathVFSHit >> -

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dexonsmith added a comment. This seems like the right direction to me! Especially like the look-through-the-ErrorInfo change for `FileError` -- I hit this before and found it annoying. IMO, it'd be valuable to split out the consequential functional changes: - Improvements/changes you made to F

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dexonsmith added a comment. In D109345#2990527 , @dblaikie wrote: > (were there other regressions I mentioned/should think about?) I don't have specific concerns; I was just reading between the lines of your description... >> 1. Add `using MemoryBuffer

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dexonsmith added a comment. In D109345#2990612 , @dblaikie wrote: > Given the amount of churn this means, though - reckon it's worth it? Reckon > it needs more llvm-dev thread/buy-in/etc? I think the churn is worth since my intuition is that it has high

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dexonsmith added a comment. In D109345#2992274 , @dexonsmith wrote: > 4. One or more commits: > 1. Migrate in-tree callers to MemoryBuffer. > 2. Delete MemoryBufferErrorAPI alias. > 5. Delete MemoryBufferErrorCodeAPI wrappers. (Potentially MemoryBuf

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dexonsmith added a comment. In D109345#3008426 , @dblaikie wrote: > Thanks for the suggestions/details, @dexonsmith - I've posted to llvm-dev > here: https://groups.google.com/g/llvm-dev/c/m9UVRhzJvh4/m/qdd_SyPuCQAJ and > will wait for some follow-up (

[Lldb-commits] [PATCH] D111454: Move TargetRegistry.(h|cpp) from Support to MC

dexonsmith added a comment. Seems like a reasonable move to me. Comment at: llvm/include/llvm/MC/TargetRegistry.h:18-19 #ifndef LLVM_SUPPORT_TARGETREGISTRY_H #define LLVM_SUPPORT_TARGETREGISTRY_H Should be `LLVM_MC_TARGETREGISTRY_H` now. ===

[Lldb-commits] [PATCH] D111454: Move TargetRegistry.(h|cpp) from Support to MC

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/D111454/new/ https://reviews.llvm.org/D111454 __

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

dexonsmith added a comment. Given how large this is, would it be reasonable to split this up a bit more? What I might do if this were my patch: get a review of the API change + the manual changes in one patch (assuming there aren't many manual changes), then land the remaining mechanical change

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, once @MaskRay is happy. I have a couple of minor comments inline too. (I also see that there are some clang-format suggestions in the unit tests; not sure any of them are actuall

[Lldb-commits] [PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

dexonsmith added a comment. In D104819#2839315 , @mstorsjo wrote: > In D104819#2839263 , @dexonsmith > wrote: > >> Just be sure to clang-format when you do the mechanical changes in the >> follow up patches.) >

[Lldb-commits] [PATCH] D92957: ExpressionParser: Migrate to FileEntryRef in ParseInternal, NFC

dexonsmith created this revision. Herald added a subscriber: ributzka. dexonsmith requested review of this revision. Herald added a project: LLDB. Migrate to the `FileEntryRef` overload of `SourceManager::createFileID` (using `FileManager::getOptionalFileRef`) in `ClangExpressionParser::ParseInter

[Lldb-commits] [PATCH] D94811: [lldb] Fix fallthrough with strictly virtual working directory.

dexonsmith added a comment. I'm wondering about this: > Currently, calling setCurrentWorkingDirectory on RedirectingFileSystem > will attempt to change the working directory for the external FS and if > that fails, it will set ExternalFSValidWD to false which prevents > fallthrough. I'm worried

[Lldb-commits] [PATCH] D94811: [lldb] Fix fallthrough with strictly virtual working directory.

dexonsmith added a comment. I missed a few words: In D94811#2502192 , @dexonsmith wrote: > What if someone does `setWorkingDirectory()` for `/a/b/c/d.c` followed by "changing directory to" > `/x`, and then looks up the relative path `d.c`? Or changes di

[Lldb-commits] [PATCH] D94811: [lldb] Fix fallthrough with strictly virtual working directory.

dexonsmith added a comment. In D94811#2502504 , @JDevlieghere wrote: > Personally I'd just like to get rid of `ExternalFSValidWD` inside of > `shouldUseExternalFS` and let the underlying FS take care of it, which I > believe would better fit the abstrac

[Lldb-commits] [PATCH] D92957: ExpressionParser: Migrate to FileEntryRef in ParseInternal, NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8b6aedc4c99a: ExpressionParser: Migrate to FileEntryRef in ParseInternal, NFC (authored by dexonsmith). Repository: rG LLVM Github Monorepo CHANG