[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

2022-09-08 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. See `test/tools/llvm-objcopy/ELF/compress-debug-sections-zstd.test`. Use a similar file for ELFCLASS32 which runs `llvm-objcopy --compress-debug-sections=zstd` then `llvm-objcopy --decompress-debug-sections`. Then compare the output with the original with just one

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

2022-09-08 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:509 + const ElfType OutputElfType = + getOutputElfType(Config.OutputArch.value_or(MachineInfo())); + const bool Is64Bit = This is incorrect. Config.OutputArch is usually unset

[Lldb-commits] [PATCH] D133530: [lldb] Add zstd support

2022-09-08 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: lldb/test/Shell/ObjectFile/ELF/compressed-sections-zstd.yaml:19 +Content: deadbeefbaadf00d +## The legacy .zdebug format is not supported. + - Name:.zdebug_info Delete `.zdebug`. It is unrelated

[Lldb-commits] [PATCH] D132300: [clang][lldb][cmake] Use new `*_INSTALL_LIBDIR_BASENAME` CPP macro

2022-09-15 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In the long term we should just remove the `CLANG_INSTALL_LIBDIR_BASENAME` customization. This is supposed for GCC multilib lib32 lib64 names but we don't necessarily use it for Clang + compiler-rt files. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D133525: fix extra bytes when compressing for 32bit objcopy

2022-09-21 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. D134385 should fix the problem:) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133525/new/ https://reviews.llvm.org/D133525 ___ lldb-commits

[Lldb-commits] [PATCH] D135400: [clang][LTO] Remove the use of `--` for arange option

2022-10-06 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. I missed https://reviews.llvm.org/D134668 . See my comment there I don't think the change is necessary. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135400/new/ https://reviews.llvm.org/D135400 ___ lldb-commits

[Lldb-commits] [PATCH] D135400: [clang][LTO] Remove the use of `--` for arange option

2022-10-06 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/Driver/debug-options-aranges.c:6 // RUN: %clang -### -g --target=x86_64-linux -flto=thin -gdwarf-aranges %s 2>&1 | FileCheck %s -// CHECK:

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

2022-08-08 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay created this revision. MaskRay added reviewers: JDevlieghere, kastiglione. Herald added a subscriber: StephenFan. Herald added a project: All. MaskRay requested review of this revision. Herald added projects: clang, LLDB. Herald added subscribers: lldb-commits, cfe-commits. The header

[Lldb-commits] [PATCH] D131422: [lldb] Remove include/lldb/lldb-private.h

2022-08-08 Thread Fangrui Song via Phabricator via lldb-commits
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGcdeb50c32155: [lldb] Remove include/lldb/lldb-private.h (authored by MaskRay). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

2022-08-09 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D130689#3710291 , @aaron.ballman wrote: > In D130689#3710281 , @royjacobson > wrote: > >> In D130689#3709834 , @thieta wrote: >> >>> In

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

2022-08-06 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D130689#3704581 , @dyung wrote: > Your change is causing a build failure on the PS4 linux build bot using GCC > 9.3. Can you take a look? > https://lab.llvm.org/buildbot/#/builders/139/builds/26186 > > FAILED: >

[Lldb-commits] [PATCH] D140999: [NFC][TargetParser] Deprecate llvm/Support/AArch64TargetParser.h

2023-01-05 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. I don't think it is necessary to deprecate the old header then delete it after 16.0.0 is branched. llvm/Support/AArch64TargetParser.h has very few open-source out-of-tree uses. Perhaps only ldc `driver/targetmachine.cpp` uses the header.

[Lldb-commits] [PATCH] D140896: [WIP] Move from llvm::makeArrayRef to ArrayRef deduction guides

2023-01-03 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. LGTM if `makeArrayRef` is kept in this patch. Do we foresee compiler bugs (for supported compilers) in this area? If there may be a risk, consider migrate a part of the code base first. CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:48 + +constexpr int NoCompression = -5; +constexpr int BestSpeedCompression = 1; MaskRay wrote: > I missed the values here. Why is -5 picked for NoCompression? What does it >

[Lldb-commits] [PATCH] D137503: [CMake] Fix -Wstrict-prototypes

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D137503#3910651 , @aaron.ballman wrote: > LGTM, thank you for this! If we do a 15.0.5, I think these changes should > probably go into there as well (WDYT?) I support this. This will make llvm-project 15.0.5 buildable by

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. Herald added a subscriber: Moerafaat. > We held off on this before as `LLVM_LIBDIR_SUFFIX` conflicted with it. Now we > return this. > > I imagine this is too potentially-breaking to make LLVM 15. That's fine. ... These sentences are no longer relevant and should be

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. zstd provides GNU Makefile, CMake, and Meson. The CMake files are only installed when configured with CMake. Debian and Ubuntu lack such files. The pkg-config file libzstd.pc is probably the most portable file. (I have also used it for a binutils-gdb patch.) I think we

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

2022-12-06 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. I am still seeing the zstdConfig.cmake zstd-config.cmake warning. @ckissane @phosek will you fix it? :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

2022-12-06 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:48 + +constexpr int NoCompression = -5; +constexpr int BestSpeedCompression = 1; I missed the values here. Why is -5 picked for NoCompression? What does it mean? zstd.h says

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

2022-12-06 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Does this address the macOS build failure? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. The name `llvm/lib/TargetParser` LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137838/new/ https://reviews.llvm.org/D137838 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. Please check these builds all work: - default - `-DLLVM_LINK_LLVM_DYLIB=on` - `-DBUILD_SHARED_LIBS=on` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137838/new/

[Lldb-commits] [PATCH] D137724: [CMake] Warn when the version is older than 3.20.0.

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision as: MaskRay. MaskRay added a comment. I think `if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)` checks for standalone builds is not necessary. The check in `llvm/CMakeLists.txt` suffices. It's unlikely the users will use different cmake versions to configure

[Lldb-commits] [PATCH] D137838: [RFC][Support] Move TargetParsers to new component

2022-12-07 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. Moving target-specific parsers outside LLVMSupport LGTM. I even objected a bit when more stuff of this kind was introduced into LLVMSupport. +1 for adding temporary forwarding headers for now to avoid update all `#include` users. Repository: rG LLVM Github Monorepo

[Lldb-commits] [PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

2022-12-19 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. Can you push `using OptionalFileEntryRef = CustomizableOptional;` and the renaming as a separate commit to make this patch smaller? There is a smaller that this rename may be reverted. 205c0589f918f95d2f2c586a01bea2716d73d603

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

2022-11-23 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp:218 + explicit NameLookup(std::nullopt_t) : Data(nullptr, true) {} explicit NameLookup(std::nullptr_t) : Data(nullptr, false) {} NameLookup() : NameLookup(nullptr) {}

[Lldb-commits] [PATCH] D138376: Use None consistently (NFC)

2022-11-19 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. `enum class NoneType { None = 1 };` is from 0cd22f9540c0591132ec991c51103cf800cf4e24 (2017) for MSVC workaround. I assume it is no longer needed.. Repository: rG LLVM Github

[Lldb-commits] [PATCH] D138310: [NFC] Make headers self-contained.

2022-11-28 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Herald added a subscriber: StephenFan. Comment at: lldb/source/Plugins/Instruction/RISCV/RISCVInstructions.h:15 -#include "EmulateInstructionRISCV.h" #include

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

2022-11-22 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D137217#3945366 , @glandium wrote: > Almost there, but not quite: > > [task 2022-11-22T23:55:36.341Z] > /builds/worker/fetches/llvm-project/llvm/tools/gold/gold-plugin.cpp:1106:6: > error: no matching function for call to

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

2022-11-15 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D137217#3926601 , @tejohnson wrote: > @MaskRay wondering if this is a good change to make for ELF as well, wdyt? Yes, I think this is a good idea and improves debuggability. The change is non-trivial so so this patch

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

2022-11-15 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: lld/COFF/LTO.cpp:229 +StringRef ltoObjName; +if (bitcodeFilePath == "ld-temp.o") { + ltoObjName = MaskRay wrote: > tejohnson wrote: > > zequanwu wrote: > > > tejohnson wrote: > > > > This case should always

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

2022-11-19 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:846 + auto AddStream = [&](size_t Task, + Twine ModuleName) -> std::unique_ptr { int FD = -1; `Twine` should only be used as const

[Lldb-commits] [PATCH] D138276: TableGen: require tablegen cl::opts to be registered explicitly

2022-11-19 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Herald added a subscriber: StephenFan. This is similar to `mlir::register*Options` and looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

2023-01-30 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. To enable smooth hash function migration in the future, we can explore the idea of adding `#ifdef EXPENSIVE_CHECKS\nshuffle` (see https://github.com/llvm/llvm-project/issues/34483). That means we should fix these tests properly to be non-dependent on the iteration

[Lldb-commits] [PATCH] D125860: [clang] Only use major version in resource dir

2022-11-04 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. Considering https://discourse.llvm.org/t/should-we-continue-embed-the-full-llvm-version-in-lib-clang/62094 and this thread, I think overall people favor this patch. If a

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

2023-03-02 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D138539#4164313 , @steakhal wrote: > Oh, I forgot to mention why this change breaks the equality operator of > `llvm::StringSet`. It's because the `StringMap::operator==` will try to > compare the `value` as well, which is

[Lldb-commits] [PATCH] D148384: [Demangle] replace use of llvm::StringView w/ std::string_view

2023-04-14 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:19 +#include + chapuni wrote: > Odd layering... Good catch. LLVMDemangle cannot depend on other LLVM libraries. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D148384: [Demangle] replace use of llvm::StringView w/ std::string_view

2023-04-14 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D148384#4270505 , @dhoekwater wrote: > It looks like this breaks the build: > https://lab.llvm.org/buildbot#builders/119/builds/12865 > https://lab.llvm.org/buildbot#builders/123/builds/18388 >

[Lldb-commits] [PATCH] D132608: [CMake] Clean up CMake binary dir handling

2023-04-12 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. Herald added subscribers: bviyer, ekilmer, jplehr, thopre. @Ericson2314 @phosek What's the state of this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132608/new/ https://reviews.llvm.org/D132608

[Lldb-commits] [PATCH] D143652: [lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing

2023-02-10 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. This seems to cause many lldb failures https://lab.llvm.org/buildbot/#/builders/68/builds/47790 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143652/new/ https://reviews.llvm.org/D143652

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-01-31 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable `CLANG_RESOURCE_DIR` but did not explain why. The patch introduced conditions in C++ code like std::string path_to_clang_dir = std::string(CLANG_RESOURCE_DIR).empty()

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

2023-02-04 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added inline comments. Comment at: llvm/test/ObjectYAML/Offload/multiple_members.yaml:21 Value:"gfx908" Content: "cafefeed" Rebase:) I fixed obj2yaml Comment at:

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

2023-02-02 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: mlir/test/Transforms/print-op-graph.mlir:1 // RUN: mlir-opt -allow-unregistered-dialect -mlir-elide-elementsattrs-if-larger=2 -view-op-graph %s -o %t

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

2023-02-02 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. > `For posterity, the tests that fail on main are:` > > `Skipped : 43` > [...] You can remove these from the description. They are flaky tests unrelated to this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

2023-02-02 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:638 - EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc"), testPath("A.h"))); + EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.h"), testPath("A.cc"))); // Make

[Lldb-commits] [PATCH] D145574: [lldb] Read register fields from target XML

2023-04-13 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: lldb/include/lldb/lldb-private-types.h:14 +#include "lldb/Target/RegisterFlags.h" #include "lldb/lldb-private.h" Is there a library layering violation? I assume that `lldb/Target/*.h` files depend on

[Lldb-commits] [PATCH] D149784: [Damangle] convert rustDemangle to use std::string_view

2023-05-03 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a subscriber: labath. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: lldb/include/lldb/Utility/ConstString.h:186 operator llvm::StringRef() const { return GetStringRef(); } + //

[Lldb-commits] [PATCH] D154542: Further refinements on reporting interruption in lldb

2023-07-06 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. Hi! The added lines have some spaces at line ends. I think using `clang-format-diff.py` would remove them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154542/new/ https://reviews.llvm.org/D154542

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

2023-07-10 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D151683#4486321 , @aaron.ballman wrote: > Aside from the failing precommit CI test case in C, I think this LGTM. I've > added @MaskRay as the code owner for the command line option changes in case > he's got concerns

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

2023-07-17 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. Reverse ping @philnik :) The `release/17.x` branch will be created soon (https://discourse.llvm.org/t/llvm-17-0-0-release-planning-and-update/71762). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151683/new/

[Lldb-commits] [PATCH] D155018: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 5)

2023-07-11 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. Still, would be good to land the lldb commit separately. And ensure that you have performed the last-minute testing... Ensure that projects like `flang bolt` still build. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D155178: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 6)

2023-07-13 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added subscribers: mysterymath, phosek. MaskRay added a comment. In D155178#4498911 , @thakis wrote: > http://45.33.8.238/linux/112305/step_4.txt looks like a missing include I've fixed it in 816141ce0eb899178dbcb6f0671875eb825b2f84

[Lldb-commits] [PATCH] D155178: [Support] Move StringExtras.h include from Error.h to Error.cpp (part 6)

2023-07-13 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D155178#4499105 , @MaskRay wrote: > In D155178#4498911 , @thakis wrote: > >> http://45.33.8.238/linux/112305/step_4.txt looks like a missing include > > I've fixed it in

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

2023-06-01 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: llvm/lib/Demangle/Demangle.cpp:49 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string ) { + if (!MangledName) +return false; Why is this change? The original contract is that `MangledName` must be

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

2023-06-02 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: llvm/lib/Demangle/Demangle.cpp:49 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string ) { + if (!MangledName) +return false; nickdesaulniers wrote: > nickdesaulniers wrote: > > MaskRay wrote: > >

[Lldb-commits] [PATCH] D149784: [Damangle] convert rustDemangle to use std::string_view

2023-06-02 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: llvm/lib/Demangle/RustDemangle.cpp:150 -char *llvm::rustDemangle(const char *MangledName) { - if (MangledName == nullptr) -return nullptr; I see that this patch drops `if (MangledName == nullptr)` (D101444). This

[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

2023-06-02 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. If `nonMicrosoftDemangle` is going to be changed to take a `string_view` argument, adding `if (!MangledName) return false;` should be fine. If possible, it's best for the change to be

[Lldb-commits] [PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

2023-05-24 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. Herald added a subscriber: JDevlieghere. Thank you for making another try for the treewide change (which is admittedly very painful and not many people do such work). Can you include the original patch description to the summary? That is the main part and the message

[Lldb-commits] [PATCH] D150996: LLVM_FALLTHROUGH => [[fallthrough]]. NFC

2023-05-24 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150996/new/ https://reviews.llvm.org/D150996

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-06-09 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D141907#4094748 , @MaskRay wrote: > [...] > edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable > `CLANG_RESOURCE_DIR` but did not explain why. > In the long term, the CMake variable `CLANG_RESOURCE_DIR`

[Lldb-commits] [PATCH] D151683: [clang] Enable C++11-style attributes in all language modes

2023-06-09 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D151683#4409633 , @aaron.ballman wrote: > In D151683#4384017 , @erichkeane > wrote: > >> In D151683#4382408 , @philnik >> wrote: >> >>> No.

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

2023-08-02 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp:23 + +class TestMCDisasmInstanceRISCV : public testing::Test { +public: Place all classes and test methods in an anonymous namespace.

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

2023-08-03 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. I'll be away for a few days but I took a quick glance. This change looks reasonable. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157028/new/ https://reviews.llvm.org/D157028

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

2023-08-03 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. In D157028#4558724 , @jansvoboda11 wrote: > Here's an example of a patch that changes the `OPTION` macro: D157029 >

[Lldb-commits] [PATCH] D157028: [llvm] Extract common `OptTable` bits into macros

2023-08-03 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added inline comments. Comment at: lld/ELF/Driver.h:28 OPT_INVALID = 0, -#define OPTION(_1, _2, ID, _4, _5, _6, _7, _8, _9, _10, _11, _12) OPT_##ID, +#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__), #include "Options.inc" lld/wasm lld/COFF

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

2023-07-26 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. It seems that a lldb specific test is needed. Adding a new method to `llvm/include/llvm/MC/MCInstrAnalysis.h` is fine with me, though I haven't checked the semantics. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D157497: feat: Migrate isArch16Bit

2023-08-14 Thread Fangrui Song via Phabricator via lldb-commits
MaskRay added a comment. In D157497#4584621 , @Pivnoy wrote: > This discussion was the main motivation for this change. > https://discourse.llvm.org/t/rfc-refactor-triple-related-classes/70410/11 > As a result, Triple should become a data class, and its

<    1   2   3