[PATCH] D110827: [LLDB] Provide target specific directories to libclang

[Raphael Isemann via Phabricator via llvm-commits]

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