[clang] Adding an API to create a CXStringSet from a Vector of StringRefs

[qiongsiwu]

The newly added API avoids converting StringRefs to std::strings when the input is a vector of StringRef.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qiongsi Wu (qiongsiwu)

Changes

The newly added API avoids converting StringRefs to std::strings when the input is a vector of StringRef.

---

Full diff: https://github.com/llvm/llvm-project/pull/136773.diff

2 Files Affected:

• (modified) clang/tools/libclang/CXString.cpp (+9-1)
• (modified) clang/tools/libclang/CXString.h (+3)

diff --git a/clang/tools/libclang/CXString.cpp b/clang/tools/libclang/CXString.cpp
index aaa8f8eeb67a1..7e46fcc3b8e69 100644
--- a/clang/tools/libclang/CXString.cpp
+++ b/clang/tools/libclang/CXString.cpp
@@ -107,7 +107,8 @@ CXString createCXString(CXStringBuf *buf) {
   return Str;
 }
 
-CXStringSet *createSet(const std::vector<std::string> &Strings) {
+template <typename StringTy>
+static CXStringSet *createSetImpl(const std::vector<StringTy> &Strings) {
   CXStringSet *Set = new CXStringSet;
   Set->Count = Strings.size();
   Set->Strings = new CXString[Set->Count];
@@ -116,6 +117,13 @@ CXStringSet *createSet(const std::vector<std::string> &Strings) {
   return Set;
 }
 
+CXStringSet *createSet(const std::vector<std::string> &Strings) {
+  return createSetImpl(Strings);
+}
+
+CXStringSet *createSet(const std::vector<StringRef> &Strings) {
+  return createSetImpl(Strings);
+}
 
 //===----------------------------------------------------------------------===//
 // String pools.
diff --git a/clang/tools/libclang/CXString.h b/clang/tools/libclang/CXString.h
index 809bdec3d677f..24c4092a9a2c0 100644
--- a/clang/tools/libclang/CXString.h
+++ b/clang/tools/libclang/CXString.h
@@ -67,7 +67,10 @@ CXString createRef(std::string String) = delete;
 /// Create a CXString object that is backed by a string buffer.
 CXString createCXString(CXStringBuf *buf);
 
+/// Create a CXStringSet object owns the strings. Such an object should be
+/// disposed with clang_disposeStringSet.
 CXStringSet *createSet(const std::vector<std::string> &Strings);
+CXStringSet *createSet(const std::vector<StringRef> &Strings);
 
 /// A string pool used for fast allocation/deallocation of strings.
 class CXStringPool {

[jansvoboda11]

LGTM, thanks! I left a couple of comments for your consideration, but don't feel obligated to incorporate them.

[AaronBallman]

Where is this new API expected to be used?

[qiongsiwu]

Where is this new API expected to be used?

An intended use can be found here swiftlang#10524, where we try to pass up a collection of strings to the user, and we try not to create copies of the strings.

[AaronBallman]

Where is this new API expected to be used?

An intended use can be found here swiftlang#10524, where we try to pass up a collection of strings to the user, and we try not to create copies of the strings.

Okay, thanks! Given there are no in-tree uses, this really needs some test coverage so nobody comes along and removes the API as being unused (it's part of the internal APIs, not the external ones).

[qiongsiwu]

Where is this new API expected to be used?

An intended use can be found here swiftlang#10524, where we try to pass up a collection of strings to the user, and we try not to create copies of the strings.

Okay, thanks! Given there are no in-tree uses, this really needs some test coverage so nobody comes along and removes the API as being unused (it's part of the internal APIs, not the external ones).

Ah good point! Makes sense to me. I did not find a straightforward place to piggy-back on to add a test (e.g. the c-index tests do not seem quite fitting). I will go find a way to add a unit test. Does this sound reasonable? Adding a unit test also feels a bit weird, since I am not seeing that we test for c interfaces in clang unit tests at a glance, but I might be missing some examples when looking.

[jansvoboda11]

How about clang/unittests/libclang/LibclangTest.cpp?

[qiongsiwu]

Hi @AaronBallman ! Again I appreciate your suggestion. I ran into some complexities when adding a test. It is precisely that this is a very internal API that is giving me some trouble. Since clang/tools/libclang/CXString.h is not an exposed header, I cannot really even add it to the clang unit tests, unless I expose (some of) the header's content through a more public API (not necessarily the C-APIs).

Do you have suggestions other than landing this PR as is? One possible alternative is to move clang/tools/libclang/CXString.h into a directory in clang/include/clang/, and then we can add the unit test. The problem with this alternative is that it seems to be too big a change for what we are aiming for here.

[AaronBallman]

Hi @AaronBallman ! Again I appreciate your suggestion. I ran into some complexities when adding a test. It is precisely that this is a very internal API that is giving me some trouble. Since clang/tools/libclang/CXString.h is not an exposed header, I cannot really even add it to the clang unit tests, unless I expose (some of) the header's content through a more public API (not necessarily the C-APIs).

Do you have suggestions other than landing this PR as is? One possible alternative is to move clang/tools/libclang/CXString.h into a directory in clang/include/clang/, and then we can add the unit test. The problem with this alternative is that it seems to be too big a change for what we are aiming for here.

If the only use is in a downstream of Clang, do we need to upstream the changes at all? We could always upstream once there's an in-tree need for the functionality.

[qiongsiwu]

If the only use is in a downstream of Clang, do we need to upstream the changes at all? We could always upstream once there's an in-tree need for the functionality.

No, we don't need to upstream. I don't have a strong feeling - I am upstreaming it because functionally this belongs to the CXString related methods, and it feels weird not having it upstream. Upstreaming it also has the benefit of allowing others to take advantage of it.

Since it is hard to test let me move this downstream. We can upstream when it matures more.

[qiongsiwu]

Closing this PR since we will have the changes in swiftlang#10524.