User Details
- User Since
- Mar 4 2015, 5:44 PM (419 w, 4 d)
Today
I still don't understand the motivation behind this change. You already explicitly set CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to ONLY for Android so why do we need to change the default?
Thu, Mar 16
LGTM
Mon, Mar 13
I have just one comment related to efficiency, but if it turns out to be too difficult to address, I'd be also fine landing this change as is and addressing that issue in a follow up change.
LGTM
Sun, Mar 12
Fri, Mar 10
LGTM
Thu, Mar 9
LGTM
Ideally we would land D143052 but the 3.20 bump is getting pushed back due to buildbot issues so I'd like to land this in the meantime to unblock other cleanup.
Wed, Mar 8
I agree with @smeenai, I think we should try to come with a more maintainable and less error prone approach.
Tue, Mar 7
LGTM
LLVM_ENABLE_PROJECTS is automatically forwarded from stage 1 builds to
stage 2 builds, so setting FUCHSIA_ENABLE_LLDB has no effect on
two-stage builds.
Mon, Mar 6
Fri, Mar 3
LGTM
Wed, Mar 1
I agree, in that case let's remove C_COMPILER_LAUNCHER;CXX_COMPILER_LAUNCHER from the list of default passthrough variables which seems like a reasonable default, and provide CLANG_BOOTSTRAP_EXTRA_PASSTHROUGH so developers have a way to pass these through to the next stage if they want to.
A more generic version might be to have a list of variables to (not) passthrough.
Tue, Feb 28
LGTM
LGTM
LGTM
Sun, Feb 26
Fri, Feb 24
Thu, Feb 23
Wed, Feb 22
Thanks!
Tue, Feb 21
I missed this change so I apologize for late response, but can you elaborate on why this is needed? Clang first checks the name without architecture and if the file doesn't exist then it'll return the one with architecture unconditionally, see https://github.com/llvm/llvm-project/blob/d61a863050bb4afd22d08bbe53af1e24c0657aba/clang/lib/Driver/ToolChain.cpp#L545. Since there are no files inside https://github.com/llvm/llvm-project/tree/main/clang/test/Driver/Inputs/resource_dir_with_arch_subdir/lib/linux/arm, it should always return the path with architecture as was the case for the previous version. LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON should play no role here since Clang tests are hermetic (as in they don't rely on runtimes being built).
LGTM
LGTM
Mon, Feb 20
LGTM
LGTM
Feb 17 2023
LGTM
runtimes/cmake/Modules/WarningFlags.cmake uses target_add_compile_flags_if_supported which is defined in https://github.com/llvm/llvm-project/blob/2b2b8409e6848361577be404bd0ae47a097f0e0c/libcxx/cmake/Modules/HandleLibcxxFlags.cmake#L246. That function uses other functions defined in the same file. That file is also not explicitly included here.
Feb 15 2023
Feb 14 2023
For the existing YAML files within LLVM, we most commonly used the PascalCase for field names and less commonly lisp-case. This change uses camelCase which is unusual and unless there's a particular reason for using this scheme such as compatibility with an existing format, I'd prefer keeping the format consistent with the rest of LLVM and using PascalCase or lisp-case.
LGTM
LGTM
Feb 13 2023
LGTM
Feb 12 2023
LGTM
Feb 11 2023
Feb 7 2023
Do you know if regular expressions are necessary to cover the existing use cases? In our experience, while regular expressions are powerful, they also tend to be error prone and more difficult to reason about. Would glob patterns that are implemented by https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/GlobPattern.h be sufficient?
Feb 5 2023
LGTM
LGTM
LGTM
Feb 4 2023
LGTM
LGTM
Another alternative would be to move these classes to the Demangle library.
Feb 3 2023
LGTM but can you update the change title and description since the original one no longer matches the content of the patch.
LGTM
LGTM
LGTM