[PATCH] D105191: [Clang][OpenMP] Add support for Static Device Libraries

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Not a thorough review but comments to address.



================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1662
+  std::string archname = ArchName.str();
+  std::string gpuname = GPUArch.str();
+
----------------
Coding convention.


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1674
+        std::string(LibraryPath + "/libdevice/lib" + libname + ".a"));
+    AOBFileNames.push_back(std::string(LibraryPath + "/lib" + libname + ".a"));
+
----------------
This will accumulate more and more entries in AOBFileNames, shouldn't you clear it at the beginning?


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1684
+
+    if (FoundAOB) {
+      std::string Err;
----------------



================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1810
+  for (std::string SDL_Name : SDL_Names) {
+    //  THIS IS THE ONLY CALL TO SDLSearch
+    if (!(SDLSearch(D, DriverArgs, CC1Args, LibraryPaths, SDL_Name, ArchName,
----------------



================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1818
+  }
+}
+
----------------
Lots of places you create new std::strings for no reason, use StringRef or const std::string & to avoid that (especially in range loops).

Please no conditional like `a != ".." && a != ".." ....`. Use a compile time set or string switch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105191/new/

https://reviews.llvm.org/D105191