teemperor requested changes to this revision. teemperor added a comment. This revision now requires changes to proceed.
Apologies for the delay, this slipped out of my review queue by accident. The patch looks in general pretty much ready, I just have a few nits here and there. (Also just to clarify what this code does. It scans the support files of some (LLDB) modules and then returns a list of include paths that are needed to compile the correct libc++/libc of the target. I fixed up the class docs a bit to make this clearer). ================ Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:34 -bool CppModuleConfiguration::analyzeFile(const FileSpec &f) { +static void getTargetIncludePaths(const llvm::Triple &triple, + std::vector<std::string> &paths) { ---------------- Could this just return a `llvm::SmallVector<std::string, 2>` ? Then you can just use it directly in the for-range below without any variable. Some docs would be nice: ``` lang=c++ /// Returns the target-specific default include paths for libc. ``` ================ Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:46 + +static bool guessIncludePath(llvm::StringRef pathToFile, + llvm::StringRef pattern, llvm::StringRef &result) { ---------------- Could this just return `llvm::Optional<std::string>`? Also the parameter should technically be `path_to_file` (this file uses LLDB's `variable_naming_style` instead of the `usualNamingStyle`, even though the existing code already has some code style issues where it deviated towards LLVM's style but oh well) ================ Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:46 + +static bool guessIncludePath(llvm::StringRef pathToFile, + llvm::StringRef pattern, llvm::StringRef &result) { ---------------- teemperor wrote: > Could this just return `llvm::Optional<std::string>`? Also the parameter > should technically be `path_to_file` (this file uses LLDB's > `variable_naming_style` instead of the `usualNamingStyle`, even though the > existing code already has some code style issues where it deviated towards > LLVM's style but oh well) ``` lang=c++ /// Returns the include path matching the given pattern for the given file path (or None if the path doesn't match the pattern). ``` ================ Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:78 + + posix_dir.consume_back("c++/v1"); + llvm::SmallVector<char, 64> tmp; ---------------- ``` lang=c++ // Check if this is a target-specific libc++ include directory. ``` ================ Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:81 + return m_std_target_inc.TrySet( + (posix_dir + triple.str() + "/c++/v1").toStringRef(tmp)); } ---------------- `.str()` instead of `.toStringRef(tmp)` should also work. ================ Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h:46 + /// If valid, the per-target include path used for the std module. + SetOncePath m_std_target_inc; /// If valid, the include path to the C library (e.g. /usr/include). ---------------- Please add a comment that this is optional unlike the other members: ```/// This is an optional path only required on some systems.``` ================ Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h:51 + /// (e.g. /usr/include/x86_64-linux-gnu). + SetOncePath m_c_target_inc; /// The Clang resource include path for this configuration. ---------------- Same as above. ================ Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h:65 /// Creates a configuration by analyzing the given list of used source files. - explicit CppModuleConfiguration(const FileSpecList &support_files); + explicit CppModuleConfiguration(const FileSpecList &support_files, + const llvm::Triple &triple); ---------------- ``` lang=c++ /// The triple (if valid) is used to search for target-specific include paths. ``` ================ Comment at: lldb/unittests/Expression/CppModuleConfigurationTest.cpp:80 + libcpp_target)); } ---------------- Could you please duplicate this test and make the target-specific directory its own new test? E.g. `TEST_F(CppModuleConfigurationTest, LinuxTargetSpecificInclude)`. Just to make sure we also still support the old behaviour. ================ Comment at: lldb/unittests/Expression/CppModuleConfigurationTest.cpp:132 EXPECT_THAT(config.GetIncludeDirs(), - testing::ElementsAre(libcpp, ResourceInc(), usr)); + testing::ElementsAre(libcpp, ResourceInc(), usr, libcpp_target)); } ---------------- Same as above: both the new and old behaviour should work so just copy this into a new test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110827/new/ https://reviews.llvm.org/D110827 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits