[libclang] Replace createRef with createDup #126683

vitalybuka · 2025-02-11T06:15:24Z

Implementation of createRef is almost exact createDup.

Created using spr 1.3.4 [skip ci]

Created using spr 1.3.4

llvmbot · 2025-02-11T06:16:01Z

@llvm/pr-subscribers-clang

Author: Vitaly Buka (vitalybuka)

Changes

Implementation of createRef is almost exact createDup.

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

7 Files Affected:

(modified) clang/tools/libclang/CIndex.cpp (+6-6)
(modified) clang/tools/libclang/CIndexCodeCompletion.cpp (+2-2)
(modified) clang/tools/libclang/CIndexDiagnostic.cpp (+1-1)
(modified) clang/tools/libclang/CXComment.cpp (+12-12)
(modified) clang/tools/libclang/CXStoredDiagnostic.cpp (+2-2)
(modified) clang/tools/libclang/CXString.cpp (-13)
(modified) clang/tools/libclang/CXString.h (-6)

diff --git a/clang/tools/libclang/CIndex.cpp b/clang/tools/libclang/CIndex.cpp
index 285ac314200077d..b644e5699646cc7 100644
--- a/clang/tools/libclang/CIndex.cpp
+++ b/clang/tools/libclang/CIndex.cpp
@@ -5066,7 +5066,7 @@ CXString clang_getFileName(CXFile SFile) {
     return cxstring::createNull();
 
   FileEntryRef FEnt = *cxfile::getFileEntryRef(SFile);
-  return cxstring::createRef(FEnt.getName());
+  return cxstring::createDup(FEnt.getName());
 }
 
 time_t clang_getFileTime(CXFile SFile) {
@@ -5154,7 +5154,7 @@ CXString clang_File_tryGetRealPathName(CXFile SFile) {
     return cxstring::createNull();
 
   FileEntryRef FEnt = *cxfile::getFileEntryRef(SFile);
-  return cxstring::createRef(FEnt.getFileEntry().tryGetRealPathName());
+  return cxstring::createDup(FEnt.getFileEntry().tryGetRealPathName());
 }
 
 //===----------------------------------------------------------------------===//
@@ -9234,7 +9234,7 @@ CXString clang_Cursor_getRawCommentText(CXCursor C) {
 
   // Don't duplicate the string because RawText points directly into source
   // code.
-  return cxstring::createRef(RawText);
+  return cxstring::createDup(RawText);
 }
 
 CXString clang_Cursor_getBriefCommentText(CXCursor C) {
@@ -9250,7 +9250,7 @@ CXString clang_Cursor_getBriefCommentText(CXCursor C) {
 
     // Don't duplicate the string because RawComment ensures that this memory
     // will not go away.
-    return cxstring::createRef(BriefText);
+    return cxstring::createDup(BriefText);
   }
 
   return cxstring::createNull();
@@ -10081,7 +10081,7 @@ cxindex::Logger::~Logger() {
 }
 
 CXString clang_getBinaryOperatorKindSpelling(enum CXBinaryOperatorKind kind) {
-  return cxstring::createRef(
+  return cxstring::createDup(
       BinaryOperator::getOpcodeStr(static_cast<BinaryOperatorKind>(kind - 1)));
 }
 
@@ -10100,7 +10100,7 @@ enum CXBinaryOperatorKind clang_getCursorBinaryOperatorKind(CXCursor cursor) {
 }
 
 CXString clang_getUnaryOperatorKindSpelling(enum CXUnaryOperatorKind kind) {
-  return cxstring::createRef(
+  return cxstring::createDup(
       UnaryOperator::getOpcodeStr(static_cast<UnaryOperatorKind>(kind - 1)));
 }
 
diff --git a/clang/tools/libclang/CIndexCodeCompletion.cpp b/clang/tools/libclang/CIndexCodeCompletion.cpp
index 850c004680fd968..188255debbc4593 100644
--- a/clang/tools/libclang/CIndexCodeCompletion.cpp
+++ b/clang/tools/libclang/CIndexCodeCompletion.cpp
@@ -223,8 +223,8 @@ clang_getCompletionParent(CXCompletionString completion_string,
   CodeCompletionString *CCStr = (CodeCompletionString *)completion_string;
   if (!CCStr)
     return cxstring::createNull();
-  
-  return cxstring::createRef(CCStr->getParentContextName());
+
+  return cxstring::createDup(CCStr->getParentContextName());
 }
 
 CXString
diff --git a/clang/tools/libclang/CIndexDiagnostic.cpp b/clang/tools/libclang/CIndexDiagnostic.cpp
index 92271d9c37f862d..e4f2d63052f1ae5 100644
--- a/clang/tools/libclang/CIndexDiagnostic.cpp
+++ b/clang/tools/libclang/CIndexDiagnostic.cpp
@@ -400,7 +400,7 @@ unsigned clang_getDiagnosticCategory(CXDiagnostic Diag) {
   
 CXString clang_getDiagnosticCategoryName(unsigned Category) {
   // Kept for backward compatibility.
-  return cxstring::createRef(DiagnosticIDs::getCategoryNameFromID(Category));
+  return cxstring::createDup(DiagnosticIDs::getCategoryNameFromID(Category));
 }
   
 CXString clang_getDiagnosticCategoryText(CXDiagnostic Diag) {
diff --git a/clang/tools/libclang/CXComment.cpp b/clang/tools/libclang/CXComment.cpp
index e87efd23578f022..668bd0a1f2fd274 100644
--- a/clang/tools/libclang/CXComment.cpp
+++ b/clang/tools/libclang/CXComment.cpp
@@ -129,7 +129,7 @@ CXString clang_TextComment_getText(CXComment CXC) {
   if (!TC)
     return cxstring::createNull();
 
-  return cxstring::createRef(TC->getText());
+  return cxstring::createDup(TC->getText());
 }
 
 CXString clang_InlineCommandComment_getCommandName(CXComment CXC) {
@@ -138,7 +138,7 @@ CXString clang_InlineCommandComment_getCommandName(CXComment CXC) {
     return cxstring::createNull();
 
   const CommandTraits &Traits = getCommandTraits(CXC);
-  return cxstring::createRef(ICC->getCommandName(Traits));
+  return cxstring::createDup(ICC->getCommandName(Traits));
 }
 
 enum CXCommentInlineCommandRenderKind
@@ -180,7 +180,7 @@ CXString clang_InlineCommandComment_getArgText(CXComment CXC,
   if (!ICC || ArgIdx >= ICC->getNumArgs())
     return cxstring::createNull();
 
-  return cxstring::createRef(ICC->getArgText(ArgIdx));
+  return cxstring::createDup(ICC->getArgText(ArgIdx));
 }
 
 CXString clang_HTMLTagComment_getTagName(CXComment CXC) {
@@ -188,7 +188,7 @@ CXString clang_HTMLTagComment_getTagName(CXComment CXC) {
   if (!HTC)
     return cxstring::createNull();
 
-  return cxstring::createRef(HTC->getTagName());
+  return cxstring::createDup(HTC->getTagName());
 }
 
 unsigned clang_HTMLStartTagComment_isSelfClosing(CXComment CXC) {
@@ -212,7 +212,7 @@ CXString clang_HTMLStartTag_getAttrName(CXComment CXC, unsigned AttrIdx) {
   if (!HST || AttrIdx >= HST->getNumAttrs())
     return cxstring::createNull();
 
-  return cxstring::createRef(HST->getAttr(AttrIdx).Name);
+  return cxstring::createDup(HST->getAttr(AttrIdx).Name);
 }
 
 CXString clang_HTMLStartTag_getAttrValue(CXComment CXC, unsigned AttrIdx) {
@@ -220,7 +220,7 @@ CXString clang_HTMLStartTag_getAttrValue(CXComment CXC, unsigned AttrIdx) {
   if (!HST || AttrIdx >= HST->getNumAttrs())
     return cxstring::createNull();
 
-  return cxstring::createRef(HST->getAttr(AttrIdx).Value);
+  return cxstring::createDup(HST->getAttr(AttrIdx).Value);
 }
 
 CXString clang_BlockCommandComment_getCommandName(CXComment CXC) {
@@ -229,7 +229,7 @@ CXString clang_BlockCommandComment_getCommandName(CXComment CXC) {
     return cxstring::createNull();
 
   const CommandTraits &Traits = getCommandTraits(CXC);
-  return cxstring::createRef(BCC->getCommandName(Traits));
+  return cxstring::createDup(BCC->getCommandName(Traits));
 }
 
 unsigned clang_BlockCommandComment_getNumArgs(CXComment CXC) {
@@ -246,7 +246,7 @@ CXString clang_BlockCommandComment_getArgText(CXComment CXC,
   if (!BCC || ArgIdx >= BCC->getNumArgs())
     return cxstring::createNull();
 
-  return cxstring::createRef(BCC->getArgText(ArgIdx));
+  return cxstring::createDup(BCC->getArgText(ArgIdx));
 }
 
 CXComment clang_BlockCommandComment_getParagraph(CXComment CXC) {
@@ -262,7 +262,7 @@ CXString clang_ParamCommandComment_getParamName(CXComment CXC) {
   if (!PCC || !PCC->hasParamName())
     return cxstring::createNull();
 
-  return cxstring::createRef(PCC->getParamNameAsWritten());
+  return cxstring::createDup(PCC->getParamNameAsWritten());
 }
 
 unsigned clang_ParamCommandComment_isParamIndexValid(CXComment CXC) {
@@ -313,7 +313,7 @@ CXString clang_TParamCommandComment_getParamName(CXComment CXC) {
   if (!TPCC || !TPCC->hasParamName())
     return cxstring::createNull();
 
-  return cxstring::createRef(TPCC->getParamNameAsWritten());
+  return cxstring::createDup(TPCC->getParamNameAsWritten());
 }
 
 unsigned clang_TParamCommandComment_isParamPositionValid(CXComment CXC) {
@@ -346,7 +346,7 @@ CXString clang_VerbatimBlockLineComment_getText(CXComment CXC) {
   if (!VBL)
     return cxstring::createNull();
 
-  return cxstring::createRef(VBL->getText());
+  return cxstring::createDup(VBL->getText());
 }
 
 CXString clang_VerbatimLineComment_getText(CXComment CXC) {
@@ -354,7 +354,7 @@ CXString clang_VerbatimLineComment_getText(CXComment CXC) {
   if (!VLC)
     return cxstring::createNull();
 
-  return cxstring::createRef(VLC->getText());
+  return cxstring::createDup(VLC->getText());
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/clang/tools/libclang/CXStoredDiagnostic.cpp b/clang/tools/libclang/CXStoredDiagnostic.cpp
index 6fb3050f5f8445e..243e55f98a7dc5a 100644
--- a/clang/tools/libclang/CXStoredDiagnostic.cpp
+++ b/clang/tools/libclang/CXStoredDiagnostic.cpp
@@ -46,7 +46,7 @@ CXSourceLocation CXStoredDiagnostic::getLocation() const {
 }
 
 CXString CXStoredDiagnostic::getSpelling() const {
-  return cxstring::createRef(Diag.getMessage());
+  return cxstring::createDup(Diag.getMessage());
 }
 
 CXString CXStoredDiagnostic::getDiagnosticOption(CXString *Disable) const {
@@ -75,7 +75,7 @@ unsigned CXStoredDiagnostic::getCategory() const {
 
 CXString CXStoredDiagnostic::getCategoryText() const {
   unsigned catID = DiagnosticIDs::getCategoryNumberForDiag(Diag.getID());
-  return cxstring::createRef(DiagnosticIDs::getCategoryNameFromID(catID));
+  return cxstring::createDup(DiagnosticIDs::getCategoryNameFromID(catID));
 }
 
 unsigned CXStoredDiagnostic::getNumRanges() const {
diff --git a/clang/tools/libclang/CXString.cpp b/clang/tools/libclang/CXString.cpp
index aaa8f8eeb67a12c..76e68fcf7a8e19b 100644
--- a/clang/tools/libclang/CXString.cpp
+++ b/clang/tools/libclang/CXString.cpp
@@ -77,19 +77,6 @@ CXString createDup(const char *String) {
   return Str;
 }
 
-CXString createRef(StringRef String) {
-  if (!String.data())
-    return createNull();
-
-  // If the string is empty, it might point to a position in another string
-  // while having zero length. Make sure we don't create a reference to the
-  // larger string.
-  if (String.empty())
-    return createEmpty();
-
-  return createDup(String);
-}
-
 CXString createDup(StringRef String) {
   CXString Result;
   char *Spelling = static_cast<char *>(llvm::safe_malloc(String.size() + 1));
diff --git a/clang/tools/libclang/CXString.h b/clang/tools/libclang/CXString.h
index 809bdec3d677ffc..54c9293e0483e89 100644
--- a/clang/tools/libclang/CXString.h
+++ b/clang/tools/libclang/CXString.h
@@ -46,12 +46,6 @@ CXString createRef(const char *String);
 /// \p String can be changed or freed by the caller.
 CXString createDup(const char *String);
 
-/// Create a CXString object from a StringRef.  New CXString may
-/// contain a pointer to the undrelying data of \p String.
-///
-/// \p String should not be changed by the caller afterwards.
-CXString createRef(StringRef String);
-
 /// Create a CXString object from a StringRef.  New CXString will
 /// contain a copy of \p String.
 ///

efriedma-quic

LGTM. Looking at the code, none of these codepaths should be hot, so I don't expect any significant performance difference.

Created using spr 1.3.4 [skip ci]

Created using spr 1.3.4

Created using spr 1.3.4 [skip ci]

Created using spr 1.3.4

Created using spr 1.3.4 [skip ci]

Created using spr 1.3.4

Created using spr 1.3.4 [skip ci]

Created using spr 1.3.4

egorzhdan

I'm not an expert in the area, but this change LGTM.

erichkeane · 2025-02-19T16:44:37Z

clang/tools/libclang/CXString.cpp

-  // If the string is empty, it might point to a position in another string
-  // while having zero length. Make sure we don't create a reference to the
-  // larger string.
-  if (String.empty())


Why is losing the above two checks not problematic? It isn't clear to me actually what this is even for, @AaronBallman I think knows more about libclang than I do though, so perhaps he should review this.

We're not losing those checks -- createDup() has them:

llvm-project/clang/tools/libclang/CXString.cpp

Line 67 in 8337d01

CXString createDup(const char *String) {

Thats only for the const char* version. Isn't the version on line 93/80 the one that will be called instead?

Oh, I see what you mean, that is a difference in behavior for the null case because clang_getCString() (part of the stable API) would start returning a non-null pointer where it previously returned a null pointer. I'm not certain if any of the changed calls to createDup() will get a null StringRef though, that's a pretty odd beast.

AaronBallman

LGTM

AaronBallman · 2025-02-19T19:16:41Z

clang/tools/libclang/CXString.cpp

-  // If the string is empty, it might point to a position in another string
-  // while having zero length. Make sure we don't create a reference to the
-  // larger string.
-  if (String.empty())


We're not losing those checks -- createDup() has them:

llvm-project/clang/tools/libclang/CXString.cpp

Line 67 in 8337d01

CXString createDup(const char *String) {

AaronBallman

Requesting changes just to be sure none of the changes from createRef() to createDup() would have generated a StringRef holding a null pointer. I'd be surprised if they did, but a second set of eyes would be helpful.

vitalybuka · 2025-02-25T23:04:47Z

Requesting changes just to be sure none of the changes from createRef() to createDup() would have generated a StringRef holding a null pointer. I'd be surprised if they did, but a second set of eyes would be helpful.

I am not sure how to proceed here.
This change was requested by @efriedma-quic on #125020

I see 3 options:

Land as-is, but then we would not differentiate createNull() vs createEmpty() vs createDup(). No idea if this difference is important.
Move checks into createDup(), but then createDup() is not always a "Dup". Also no idead if this is important.
Just abandon the patch.

WDYT?

erichkeane · 2025-02-26T00:03:12Z

Requesting changes just to be sure none of the changes from createRef() to createDup() would have generated a StringRef holding a null pointer. I'd be surprised if they did, but a second set of eyes would be helpful.

I am not sure how to proceed here. This change was requested by @efriedma-quic on #125020

I see 3 options:
1. Land as-is, but then we would not differentiate `createNull()` vs `createEmpty()` vs `createDup()`. No idea if this difference is important.

2. Move checks into `createDup()`, but then `createDup()` is not always a "Dup". Also no idead if this is important.

3. Just abandon the patch.
WDYT?

Aaron is away this week, but: I believe he was asking for an analysis of all of the uses to make sure they don't use a null-StringRef anywhere.

IMO, an assert of some sort in the createDup interface would be sufficient to long-term prevent the problem.

vitalybuka added 2 commits February 10, 2025 22:15

[𝘀𝗽𝗿] changes to main this commit is based on

9a801de

Created using spr 1.3.4 [skip ci]

[𝘀𝗽𝗿] initial version

a8bc4e1

Created using spr 1.3.4

llvmbot added clang Clang issues not falling into any other category clang:as-a-library libclang and C++ API labels Feb 11, 2025

vitalybuka mentioned this pull request Feb 11, 2025

[libclang] Always Dup in createRef(StringRef) #125020

Merged

vitalybuka requested a review from egorzhdan February 11, 2025 06:15

vitalybuka requested a review from efriedma-quic February 11, 2025 06:16

efriedma-quic requested a review from erichkeane February 11, 2025 17:20

efriedma-quic approved these changes Feb 11, 2025

View reviewed changes

vitalybuka added 8 commits February 12, 2025 22:01

[𝘀𝗽𝗿] changes introduced through rebase

c5e9141

Created using spr 1.3.4 [skip ci]

relnote

f4d53b4

Created using spr 1.3.4

[𝘀𝗽𝗿] changes introduced through rebase

70dec50

Created using spr 1.3.4 [skip ci]

quotes

69c7eb0

Created using spr 1.3.4

[𝘀𝗽𝗿] changes introduced through rebase

b809fd1

Created using spr 1.3.4 [skip ci]

the

e36e6d7

Created using spr 1.3.4

[𝘀𝗽𝗿] changes introduced through rebase

fc84c46

Created using spr 1.3.4 [skip ci]

rebase

41d8f8b

Created using spr 1.3.4

vitalybuka changed the base branch from users/vitalybuka/spr/main.libclang-replace-createref-with-createdup to main February 13, 2025 06:06

egorzhdan approved these changes Feb 13, 2025

View reviewed changes

erichkeane reviewed Feb 19, 2025

View reviewed changes

AaronBallman approved these changes Feb 19, 2025

View reviewed changes

AaronBallman requested changes Feb 19, 2025

View reviewed changes

vitalybuka requested review from AaronBallman, efriedma-quic and erichkeane February 25, 2025 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libclang] Replace createRef with createDup #126683

[libclang] Replace createRef with createDup #126683

vitalybuka commented Feb 11, 2025

llvmbot commented Feb 11, 2025

efriedma-quic left a comment

egorzhdan left a comment

erichkeane Feb 19, 2025

AaronBallman Feb 19, 2025

erichkeane Feb 19, 2025

AaronBallman Feb 19, 2025

AaronBallman left a comment

AaronBallman Feb 19, 2025

AaronBallman left a comment

vitalybuka commented Feb 25, 2025

erichkeane commented Feb 26, 2025

[libclang] Replace createRef with createDup #126683

Are you sure you want to change the base?

[libclang] Replace createRef with createDup #126683

Conversation

vitalybuka commented Feb 11, 2025

llvmbot commented Feb 11, 2025

efriedma-quic left a comment

Choose a reason for hiding this comment

egorzhdan left a comment

Choose a reason for hiding this comment

erichkeane Feb 19, 2025

Choose a reason for hiding this comment

AaronBallman Feb 19, 2025

Choose a reason for hiding this comment

erichkeane Feb 19, 2025

Choose a reason for hiding this comment

AaronBallman Feb 19, 2025

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman Feb 19, 2025

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

vitalybuka commented Feb 25, 2025

erichkeane commented Feb 26, 2025