[PATCH] D150910: [libclang] Add CXBinaryOperatorKind and CXUnaryOperatorKind (implements 29138)

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D150910#4405536 , @MineGame159 
wrote:

> I kinda thought the undefined reference error is just something I broke on my 
> machine but guess not. Don't really know what can cause it since it can link 
> to other functions from there and the function exists.

I suspect the reason for the failed build is that you haven't updated 
`clang/tools/libclang/libclang.map`. Also `CINDEX_VERSION_MINOR` in 
`clang/include/clang-c/Index.h` needs to be incremented, unless it has been 
incremented in the targeted LLVM release version already. See e.g. 
cc929590ad305f0d068709c7c7999f5fc6118dc9 
.


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

https://reviews.llvm.org/D150910

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146275: [libclang] Fix documentation; NFC

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a reviewer: aaron.ballman.
vedgy added a comment.

Noticed the mistake while reviewing the generated documentation 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146275: [libclang] Fix documentation; NFC

[Igor Kushnir via Phabricator via cfe-commits]

vedgy created this revision.
Herald added subscribers: mikhail.ramalho, arphaman.
Herald added a project: All.
vedgy requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixes cc929590ad305f0d068709c7c7999f5fc6118dc9 
. The
CXIndexOptions::GlobalOptions data member has been replaced with the two
CXChoice data members during code review.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146275

Files:
  clang/include/clang-c/Index.h


Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -445,7 +445,9 @@
 /**
  * Sets general options associated with a CXIndex.
  *
- * This function is DEPRECATED. Set CXIndexOptions::GlobalOptions and call
+ * This function is DEPRECATED. Set
+ * CXIndexOptions::ThreadBackgroundPriorityForIndexing and/or
+ * CXIndexOptions::ThreadBackgroundPriorityForEditing and call
  * clang_createIndexWithOptions() instead.
  *
  * For example:


Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -445,7 +445,9 @@
 /**
  * Sets general options associated with a CXIndex.
  *
- * This function is DEPRECATED. Set CXIndexOptions::GlobalOptions and call
+ * This function is DEPRECATED. Set
+ * CXIndexOptions::ThreadBackgroundPriorityForIndexing and/or
+ * CXIndexOptions::ThreadBackgroundPriorityForEditing and call
  * clang_createIndexWithOptions() instead.
  *
  * For example:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145974: [libclang] Add index option to store preambles in memory

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

@aaron.ballman, can you land this revision for me please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145974

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146039: [libclang] No longer attempt to get a dependent bit-width

[Igor Kushnir via Phabricator via cfe-commits]

vedgy accepted this revision.
vedgy added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D146039

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146039: [libclang] No longer attempt to get a dependent bit-width

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:3043
+ * For example:
+ * if (clang_Cursor_isBitField(Cursor)) {
+ *   int Width = clang_getFieldDeclBitWidth(Cursor);

Surround the example with ` \code` and `\endcode` commands.



Comment at: clang/include/clang-c/Index.h:3046
+ *   if (Width != -1) {
+ * // The bit-field width is non-dependent.
+ *   }

"non-dependent" is unclear to me. How about rewording to:
```
 *   int Width = clang_getFieldDeclBitWidth(Cursor);
 *   if (Width == -1) {
 * // The bit-field width depends on a template parameter.
```
or alternatively more specific standardese:
```
 * // The bit-field width is [not] value-dependent.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146039

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Fix include order in CXType.cpp

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/tools/libclang/CXType.cpp:374
 
+unsigned clang_isBitFieldDecl(CXCursor C) {
+  using namespace cxcursor;

aaron.ballman wrote:
> vedgy wrote:
> > vedgy wrote:
> > > I just noticed the `clang_Cursor_isBitField()` function implemented [[ 
> > > https://github.com/llvm/llvm-project/commit/e822f58db4dee2ae56e306512288224979b9b5ba
> > >  | 10 years ago ]] , which returns exactly the same value as this new 
> > > function. So most changes in this commit can be reverted.
> > `clang_Cursor_isBitField()` is declared much later in //Index.h// and isn't 
> > easily discoverable. `clang_getFieldDeclBitWidth()` could benefit from a 
> > usage example. Here is how I plan to use it in KDevelop:
> > ```
> > if (clang_Cursor_isBitField(cursor)) {
> > const auto bitWidth = clang_getFieldDeclBitWidth(cursor);
> > decl->setBitWidth(bitWidth == -1 ? 
> > ClassMemberDeclaration::ValueDependentBitWidth : bitWidth);
> > }
> > ```
> > I just noticed the clang_Cursor_isBitField() function implemented 10 years 
> > ago , which returns exactly the same value as this new function. So most 
> > changes in this commit can be reverted.
> 
> Wow, good catch!
> 
> I'm going to roll this commit back in its entirety rather than do a partial 
> revert, then I'll push a new changes to add back the `isValueDependent()` bit 
> below, and rearrange the header file + add an example.
Sounds good: the minimal-fix patch can be easily cherry-picked to older 
libclang versions by distro maintainers. Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Fix include order in CXType.cpp

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/tools/libclang/CXType.cpp:374
 
+unsigned clang_isBitFieldDecl(CXCursor C) {
+  using namespace cxcursor;

vedgy wrote:
> I just noticed the `clang_Cursor_isBitField()` function implemented [[ 
> https://github.com/llvm/llvm-project/commit/e822f58db4dee2ae56e306512288224979b9b5ba
>  | 10 years ago ]] , which returns exactly the same value as this new 
> function. So most changes in this commit can be reverted.
`clang_Cursor_isBitField()` is declared much later in //Index.h// and isn't 
easily discoverable. `clang_getFieldDeclBitWidth()` could benefit from a usage 
example. Here is how I plan to use it in KDevelop:
```
if (clang_Cursor_isBitField(cursor)) {
const auto bitWidth = clang_getFieldDeclBitWidth(cursor);
decl->setBitWidth(bitWidth == -1 ? 
ClassMemberDeclaration::ValueDependentBitWidth : bitWidth);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Fix include order in CXType.cpp

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/tools/libclang/CXType.cpp:374
 
+unsigned clang_isBitFieldDecl(CXCursor C) {
+  using namespace cxcursor;

I just noticed the `clang_Cursor_isBitField()` function implemented [[ 
https://github.com/llvm/llvm-project/commit/e822f58db4dee2ae56e306512288224979b9b5ba
 | 10 years ago ]] , which returns exactly the same value as this new function. 
So most changes in this commit can be reverted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145974: [libclang] Add index option to store preambles in memory

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

I have implemented the setter for the new option locally and tested it in 
KDevelop.

  void clang_CXIndex_setStorePreamblesInMemory(CXIndex CIdx,
   int storePreamblesInMemory) {
if (CIdx)
  static_cast(CIdx)->setStorePreamblesInMemory(
  storePreamblesInMemory);
  }

Works as expected: new preambles are created in memory or the temporary 
directory depending on the option selected in the UI. Even if the temporary 
storage option ends up in memory, all remaining preambles are removed from the 
temporary directory on exit.

This simple setter approach was rejected in recent comments under D143418 
 for valid reasons. But this quick test 
proves that changing the option on the fly is viable. Currently I don't think 
that the option change taking effect without restarting KDevelop justifies the 
effort of implementing the setter as `clang_parseTranslationUnitWithOptions()`. 
I expect this option's value to be changed very rarely. So I don't plan to 
change preamble storage API after this review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145974

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Just created the follow-up `StorePreamblesInMemory` revision: D145974 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145974: [libclang] Add index option to store preambles in memory

[Igor Kushnir via Phabricator via cfe-commits]

vedgy created this revision.
Herald added a subscriber: arphaman.
Herald added a project: All.
vedgy requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This commit allows libclang API users to opt into storing PCH in memory
instead of temporary files. The option can be set only during CXIndex
construction to avoid multithreading issues and confusion or bugs if
some preambles are stored in temporary files and others - in memory.

The added API works as expected in KDevelop:
https://invent.kde.org/kdevelop/kdevelop/-/merge_requests/283


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145974

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CIndexer.h
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/libclang/LibclangTest.cpp

Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -479,6 +479,7 @@
 };
 
 class LibclangSetPreambleStoragePathTest : public LibclangPreambleStorageTest {
+  virtual bool StorePreamblesInMemory() { return false; }
   virtual const char *PreambleStoragePath() = 0;
 
 protected:
@@ -487,6 +488,7 @@
 
 CXIndexOptions Opts{};
 Opts.Size = sizeof(CXIndexOptions);
+Opts.StorePreamblesInMemory = StorePreamblesInMemory();
 Opts.PreambleStoragePath = PreambleStoragePath();
 Index = clang_createIndexWithOptions();
 ASSERT_TRUE(Index);
@@ -506,6 +508,19 @@
   const char *PreambleStoragePath() override { return PreambleDir.c_str(); }
 };
 
+class LibclangStoreInMemoryNullPreambleStoragePathTest
+: public LibclangNullPreambleStoragePathTest {
+  bool StorePreamblesInMemory() override { return true; }
+};
+class LibclangStoreInMemoryEmptyPreambleStoragePathTest
+: public LibclangEmptyPreambleStoragePathTest {
+  bool StorePreamblesInMemory() override { return true; }
+};
+class LibclangStoreInMemoryPreambleDirPreambleStoragePathTest
+: public LibclangPreambleDirPreambleStoragePathTest {
+  bool StorePreamblesInMemory() override { return true; }
+};
+
 TEST_F(LibclangNotOverriddenPreambleStoragePathTest, CountPreambles) {
   CountPreamblesInPreambleDir(0);
 }
@@ -518,6 +533,16 @@
 TEST_F(LibclangPreambleDirPreambleStoragePathTest, CountPreambles) {
   CountPreamblesInPreambleDir(1);
 }
+TEST_F(LibclangStoreInMemoryNullPreambleStoragePathTest, CountPreambles) {
+  CountPreamblesInPreambleDir(0);
+}
+TEST_F(LibclangStoreInMemoryEmptyPreambleStoragePathTest, CountPreambles) {
+  CountPreamblesInPreambleDir(0);
+}
+TEST_F(LibclangStoreInMemoryPreambleDirPreambleStoragePathTest,
+   CountPreambles) {
+  CountPreamblesInPreambleDir(0);
+}
 
 TEST_F(LibclangParseTest, AllSkippedRanges) {
   std::string Header = "header.h", Main = "main.cpp";
Index: clang/unittests/Frontend/ASTUnitTest.cpp
===
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ clang/unittests/Frontend/ASTUnitTest.cpp
@@ -167,7 +167,7 @@
   std::unique_ptr ErrUnit;
 
   ASTUnit *AST = ASTUnit::LoadFromCommandLine(
-  [0], [4], PCHContainerOps, Diags, "", "", false,
+  [0], [4], PCHContainerOps, Diags, "", false, "", false,
   CaptureDiagsKind::All, std::nullopt, true, 0, TU_Complete, false, false,
   false, SkipFunctionBodiesScope::None, false, true, false, false,
   std::nullopt, , nullptr);
Index: clang/tools/libclang/CIndexer.h
===
--- clang/tools/libclang/CIndexer.h
+++ clang/tools/libclang/CIndexer.h
@@ -34,6 +34,7 @@
 class CIndexer {
   bool OnlyLocalDecls;
   bool DisplayDiagnostics;
+  bool StorePreamblesInMemory = false;
   unsigned Options; // CXGlobalOptFlags.
 
   std::string ResourcesPath;
@@ -78,6 +79,11 @@
 
   StringRef getClangToolchainPath();
 
+  void setStorePreamblesInMemory(bool StoreInMemory) {
+StorePreamblesInMemory = StoreInMemory;
+  }
+  bool getStorePreamblesInMemory() const { return StorePreamblesInMemory; }
+
   void setPreambleStoragePath(StringRef Str) {
 PreambleStoragePath = Str.str();
   }
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3742,6 +3742,7 @@
   options->ExcludeDeclarationsFromPCH, options->DisplayDiagnostics,
   options->ThreadBackgroundPriorityForIndexing,
   options->ThreadBackgroundPriorityForEditing);
+  CIdxr->setStorePreamblesInMemory(options->StorePreamblesInMemory);
   CIdxr->setPreambleStoragePath(options->PreambleStoragePath);
   CIdxr->setInvocationEmissionPath(options->InvocationEmissionPath);
   return CIdxr;
@@ -3956,8 +3957,9 @@
   std::unique_ptr 

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:365
+   */
+  int ExcludeDeclarationsFromPCH : 1;
+  /**

aaron.ballman wrote:
> vedgy wrote:
> > vedgy wrote:
> > > aaron.ballman wrote:
> > > > vedgy wrote:
> > > > > Assigning `true` to `int : 1` bit-fields in C++ code produces a GCC 
> > > > > warning:
> > > > > ```
> > > > > warning: overflow in conversion from ‘int’ to ‘signed char:1’ changes 
> > > > > value from ‘1’ to ‘-1’ [-Woverflow]
> > > > > ```
> > > > > 
> > > > > Following a suggestion in a comment to 
> > > > > https://github.com/llvm/llvm-project/issues/53253, I replaced this 
> > > > > `int` with `unsigned` and the warning disappeared. Same for `int 
> > > > > DisplayDiagnostics : 1`. Should this type change be included in the 
> > > > > upcoming `StorePreamblesInMemory` revision?
> > > > > Assigning true to int : 1 bit-fields in C++ code produces a GCC 
> > > > > warning:
> > > > >
> > > > > `warning: overflow in conversion from ‘int’ to ‘signed char:1’ 
> > > > > changes value from ‘1’ to ‘-1’ [-Woverflow]`
> > > > 
> > > > Ugh, I forgot that the C standard allows that. (C2x 6.7.2.1p12: "A 
> > > > bit-field member is interpreted as having a signed or unsigned integer 
> > > > type consisting of the specified number of bits" -- GCC decided to turn 
> > > > our `int` into `signed char` which is nice for packing data together, 
> > > > but not as nice when it comes to boolean-like bit-fields.)
> > > > 
> > > > > Should this type change be included in the upcoming 
> > > > > StorePreamblesInMemory revision?
> > > > 
> > > > It'd probably be the cleanest to fix that separately. Given that it's 
> > > > NFC and you don't have commit privileges, I can make the change on your 
> > > > behalf and land it today if that's what you'd like.
> > > Or should this change be done in a separate revision, on which the 
> > > `StorePreamblesInMemory` would be based?
> > > 
> > > I also implemented two other changes to the `struct CXIndexOptions` 
> > > (mostly documentation/comments). Should these all be in separate 
> > > revisions or combined into one?
> > Yes, I agree that such changes should be in separate commits. But I don't 
> > want to burden you with committing them all separately. So if 4 is too 
> > much, I can request the commit access for myself. If this burden is not too 
> > heavy, I am fine with you making the change on my behalf.
> No worries, this was a trivial one -- I landed it in 
> dbde7cc17c3a5b6a35e5ec598ba7eaba6f75d90b, so you should be able to fetch and 
> rebase on top of that.
> I also implemented two other changes to the struct CXIndexOptions (mostly 
> documentation/comments).
Here they are: D145775, D145783.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145783: Reserve unused bits in struct CXIndexOptions; NFC

[Igor Kushnir via Phabricator via cfe-commits]

vedgy created this revision.
Herald added a subscriber: arphaman.
Herald added a project: All.
vedgy requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145783

Files:
  clang/include/clang-c/Index.h
  clang/tools/libclang/CIndex.cpp


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3724,6 +3724,13 @@
   // if (options->Size >= offsetof(CXIndexOptions, RecentlyAddedMember))
   //   do_something(options->RecentlyAddedMember);
 
+  // An exception: if a new option is small enough, it can be squeezed into the
+  // /*Reserved*/ bits in CXIndexOptions. Since the default value of each 
option
+  // is guaranteed to be 0 and the callers are advised to zero out the struct,
+  // programs built against older libclang versions would implicitly set the 
new
+  // options to default values, which should keep the behavior of previous
+  // libclang versions and thus be backward-compatible.
+
   // If options->Size > sizeof(CXIndexOptions), the user may have set an option
   // we can't handle, in which case we return nullptr to report failure.
   // Replace `!=` with `>` here to support older struct versions. `!=` has the
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -367,6 +367,7 @@
* \see clang_createIndex()
*/
   unsigned DisplayDiagnostics : 1;
+  unsigned /*Reserved*/ : 14;
   /**
* The path to a directory, in which to store temporary PCH files. If null or
* empty, the default system temporary directory is used. These PCH files are


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3724,6 +3724,13 @@
   // if (options->Size >= offsetof(CXIndexOptions, RecentlyAddedMember))
   //   do_something(options->RecentlyAddedMember);
 
+  // An exception: if a new option is small enough, it can be squeezed into the
+  // /*Reserved*/ bits in CXIndexOptions. Since the default value of each option
+  // is guaranteed to be 0 and the callers are advised to zero out the struct,
+  // programs built against older libclang versions would implicitly set the new
+  // options to default values, which should keep the behavior of previous
+  // libclang versions and thus be backward-compatible.
+
   // If options->Size > sizeof(CXIndexOptions), the user may have set an option
   // we can't handle, in which case we return nullptr to report failure.
   // Replace `!=` with `>` here to support older struct versions. `!=` has the
Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -367,6 +367,7 @@
* \see clang_createIndex()
*/
   unsigned DisplayDiagnostics : 1;
+  unsigned /*Reserved*/ : 14;
   /**
* The path to a directory, in which to store temporary PCH files. If null or
* empty, the default system temporary directory is used. These PCH files are
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145775: Improve documentation of CXIndexOptions; NFC

[Igor Kushnir via Phabricator via cfe-commits]

vedgy created this revision.
Herald added a subscriber: arphaman.
Herald added a project: All.
vedgy requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Document one more alternative way to initialize struct CXIndexOptions,
which is used in LibclangSetPreambleStoragePathTest since
df8f8f76207df40dca11c9c0c2328d6b3dfba9ca 
 because 
the previous
initialization approach causes compiler warnings.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145775

Files:
  clang/include/clang-c/Index.h


Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -329,8 +329,8 @@
  * Index initialization options.
  *
  * 0 is the default value of each member of this struct except for Size.
- * Initialize the struct in one of the following two ways to avoid adapting 
code
- * each time a new member is added to it:
+ * Initialize the struct in one of the following three ways to avoid adapting
+ * code each time a new member is added to it:
  * \code
  * CXIndexOptions Opts;
  * memset(, 0, sizeof(Opts));
@@ -340,6 +340,11 @@
  * \code
  * CXIndexOptions Opts = { sizeof(CXIndexOptions) };
  * \endcode
+ * or to prevent the -Wmissing-field-initializers warning for the above 
version:
+ * \code
+ * CXIndexOptions Opts{};
+ * Opts.Size = sizeof(CXIndexOptions);
+ * \endcode
  */
 typedef struct CXIndexOptions {
   /**


Index: clang/include/clang-c/Index.h
===
--- clang/include/clang-c/Index.h
+++ clang/include/clang-c/Index.h
@@ -329,8 +329,8 @@
  * Index initialization options.
  *
  * 0 is the default value of each member of this struct except for Size.
- * Initialize the struct in one of the following two ways to avoid adapting code
- * each time a new member is added to it:
+ * Initialize the struct in one of the following three ways to avoid adapting
+ * code each time a new member is added to it:
  * \code
  * CXIndexOptions Opts;
  * memset(, 0, sizeof(Opts));
@@ -340,6 +340,11 @@
  * \code
  * CXIndexOptions Opts = { sizeof(CXIndexOptions) };
  * \endcode
+ * or to prevent the -Wmissing-field-initializers warning for the above version:
+ * \code
+ * CXIndexOptions Opts{};
+ * Opts.Size = sizeof(CXIndexOptions);
+ * \endcode
  */
 typedef struct CXIndexOptions {
   /**
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy marked an inline comment as not done.
vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:365
+   */
+  int ExcludeDeclarationsFromPCH : 1;
+  /**

vedgy wrote:
> aaron.ballman wrote:
> > vedgy wrote:
> > > Assigning `true` to `int : 1` bit-fields in C++ code produces a GCC 
> > > warning:
> > > ```
> > > warning: overflow in conversion from ‘int’ to ‘signed char:1’ changes 
> > > value from ‘1’ to ‘-1’ [-Woverflow]
> > > ```
> > > 
> > > Following a suggestion in a comment to 
> > > https://github.com/llvm/llvm-project/issues/53253, I replaced this `int` 
> > > with `unsigned` and the warning disappeared. Same for `int 
> > > DisplayDiagnostics : 1`. Should this type change be included in the 
> > > upcoming `StorePreamblesInMemory` revision?
> > > Assigning true to int : 1 bit-fields in C++ code produces a GCC warning:
> > >
> > > `warning: overflow in conversion from ‘int’ to ‘signed char:1’ changes 
> > > value from ‘1’ to ‘-1’ [-Woverflow]`
> > 
> > Ugh, I forgot that the C standard allows that. (C2x 6.7.2.1p12: "A 
> > bit-field member is interpreted as having a signed or unsigned integer type 
> > consisting of the specified number of bits" -- GCC decided to turn our 
> > `int` into `signed char` which is nice for packing data together, but not 
> > as nice when it comes to boolean-like bit-fields.)
> > 
> > > Should this type change be included in the upcoming 
> > > StorePreamblesInMemory revision?
> > 
> > It'd probably be the cleanest to fix that separately. Given that it's NFC 
> > and you don't have commit privileges, I can make the change on your behalf 
> > and land it today if that's what you'd like.
> Or should this change be done in a separate revision, on which the 
> `StorePreamblesInMemory` would be based?
> 
> I also implemented two other changes to the `struct CXIndexOptions` (mostly 
> documentation/comments). Should these all be in separate revisions or 
> combined into one?
Yes, I agree that such changes should be in separate commits. But I don't want 
to burden you with committing them all separately. So if 4 is too much, I can 
request the commit access for myself. If this burden is not too heavy, I am 
fine with you making the change on my behalf.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy marked an inline comment as not done.
vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:365
+   */
+  int ExcludeDeclarationsFromPCH : 1;
+  /**

vedgy wrote:
> Assigning `true` to `int : 1` bit-fields in C++ code produces a GCC warning:
> ```
> warning: overflow in conversion from ‘int’ to ‘signed char:1’ changes value 
> from ‘1’ to ‘-1’ [-Woverflow]
> ```
> 
> Following a suggestion in a comment to 
> https://github.com/llvm/llvm-project/issues/53253, I replaced this `int` with 
> `unsigned` and the warning disappeared. Same for `int DisplayDiagnostics : 
> 1`. Should this type change be included in the upcoming 
> `StorePreamblesInMemory` revision?
Or should this change be done in a separate revision, on which the 
`StorePreamblesInMemory` would be based?

I also implemented two other changes to the `struct CXIndexOptions` (mostly 
documentation/comments). Should these all be in separate revisions or combined 
into one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:365
+   */
+  int ExcludeDeclarationsFromPCH : 1;
+  /**

Assigning `true` to `int : 1` bit-fields in C++ code produces a GCC warning:
```
warning: overflow in conversion from ‘int’ to ‘signed char:1’ changes value 
from ‘1’ to ‘-1’ [-Woverflow]
```

Following a suggestion in a comment to 
https://github.com/llvm/llvm-project/issues/53253, I replaced this `int` with 
`unsigned` and the warning disappeared. Same for `int DisplayDiagnostics : 1`. 
Should this type change be included in the upcoming `StorePreamblesInMemory` 
revision?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Please update the commit message (there is no 
`clang_isFieldDeclBitWidthDependent` anymore) and update the revision with `arc 
diff --verbatim @~`.




Comment at: clang/include/clang-c/Index.h:3552
+ * If the cursor does not reference a bit field declaration or if the bit
+ * field's width does not depend on template parameters, 0 is returned.
+ */

collinbaker wrote:
> vedgy wrote:
> > vedgy wrote:
> > > collinbaker wrote:
> > > > vedgy wrote:
> > > > > I just thought how the new API could be used in KDevelop. Currently 
> > > > > when `clang_getFieldDeclBitWidth()` is positive, e.g. 2, KDevelop 
> > > > > shows  ` : 2` after the data member name in a tooltip. Ideally a 
> > > > > template-param-dependent expression (actual code) would be displayed 
> > > > > after the colon. If that's difficult to implement, `: 
> > > > > [tparam-dependent]` or `: ?` could be displayed instead. But it would 
> > > > > be more convenient and efficient to get this information by a single 
> > > > > call to `clang_getFieldDeclBitWidth()` instead of calling 
> > > > > `clang_isFieldDeclBitWidthDependent()` each time 
> > > > > `clang_getFieldDeclBitWidth()` returns `-1`. So how about returning 
> > > > > `-2` or `0` from `clang_getFieldDeclBitWidth()` instead of adding 
> > > > > this new API?
> > > > I understand the motivation but I don't think requiring an extra call 
> > > > is asking too much of libclang clients, and it's one extra call that 
> > > > doesn't do much work and will be called rarely so I don't see 
> > > > efficiency concerns. Without strong reasons otherwise I think it's 
> > > > better to be explicit here.
> > > KDevelop calls `clang_getFieldDeclBitWidth()` for each encountered class 
> > > member declaration. `clang_isFieldDeclBitWidthDependent()` would have to 
> > > be called each time `clang_getFieldDeclBitWidth()` returns `-1`, which 
> > > would be most of the time, because few class members are bit-fields. The 
> > > work this new function does is the same as that of 
> > > `clang_getFieldDeclBitWidth()`  (repeated).
> > > 
> > > If the concern about returning `-2` from `clang_getFieldDeclBitWidth()` 
> > > is cryptic return codes, an `enum` with named constants can be introduced.
> > > 
> > > If the concern is breaking backward compatibility for users that relied 
> > > on the returned value being positive or `-1`, then a replacement for 
> > > `clang_getFieldDeclBitWidth()` with the most convenient API should be 
> > > introduced and `clang_getFieldDeclBitWidth()` itself - deprecated.
> > > 
> > > KDevelop simply stores the value returned by 
> > > `clang_getFieldDeclBitWidth()` in an `int16_t m_bitWidth` data member and 
> > > uses it later. So if `-2` is returned, the only place in code to adjust 
> > > would be the use of this data member. With the current 
> > > `clang_isFieldDeclBitWidthDependent()` implementation, this function 
> > > would have to be called, `-2` explicitly stored in `m_bitWidth` and the 
> > > use of `m_bitWidth` would have to be adjusted just the same.
> > > 
> > > Have you considered potential usage of the added API in your project? 
> > > Which alternative would be more convenient to use?
> > One more API alternative is to replace 
> > `clang_isFieldDeclBitWidthDependent()` with `clang_isBitFieldDecl()`. The 
> > usage would be more straightforward and efficient: first call 
> > `clang_isBitFieldDecl()`; if it returns true (should be rare enough), call 
> > `clang_getFieldDeclBitWidth()`; if that returns `-1`, then the bit-field 
> > width must be unknown (dependent on template parameters). Such usage would 
> > still be less convenient and efficient compared to 
> > `clang_getFieldDeclBitWidth()` returning `-2` though.
> Implemented as `clang_isBitFieldDecl` rather than magic return value
Thanks. That's good enough for me.



Comment at: clang/tools/libclang/CXType.cpp:13
 
+#include "CXType.h"
 #include "CIndexer.h"

collinbaker wrote:
> vedgy wrote:
> > I guess //clang-format// did this include reordering. But it certainly 
> > looks out of place and the include order becomes wrong. So I think it 
> > should be reverted.
> I don't agree, it's pretty standard for a source file to have its associated 
> header include at the top.
You are right, I haven't realized the header-source association. The diff is 
still unrelated to the patch. But I'm no longer sure what's right, so won't 
insist on anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4175628 , @aaron.ballman 
wrote:

> Hmmm, don't relaxed loads and stores still have the potential to be racey? I 
> thought you needed a release on the store and an acquire on the load (or 
> sequential consistency), but this is definitely not my area of expertise.

The acquire/release semantics is needed if the atomic variable guards access to 
another non-atomic variable or the atomic pointer guards access to its 
non-atomic pointed-to value. The relaxed order means no guarantees about this 
variable's interaction with other atomic or non-atomic variables. But even the 
relaxed order prevents data races on the variable itself, which is sufficient 
here.

>> The option can also be added to `CXIndexOptions` in order to allow setting 
>> its initial value reliably (not worrying whether it could be used before the 
>> setter gets called after index construction).
>>
>> Is adding both the setter and the `CXIndexOptions` member OK or would you 
>> prefer only one of these two?
>
> It's a bit odd to me that we're deprecating the global option setters to turn 
> around and add a new global option setter. The old interfaces are not thread 
> safe and the new one is thread safe, so I get why the desire exists and is 
> reasonable, but (personally) I'd like to get rid of the option state setters 
> entirely someday.

I also thought about the deprecated old interfaces today. 
`clang_CXIndex_setGlobalOptions()` could be undeprecated by similarly making 
`CIndexer::Options` atomic. Safely setting `std::string` members would require 
a mutex.

> What I was envisioning was that the index creation would get global options 
> and if we wanted per-TU options, we'd add an interface akin to 
> `clang_parseTranslationUnit()` which takes another options struct for per-TU 
> options. (Perhaps the default values for those options come from the global 
> options -- e.g., `DisplayDiagnostics` is set at the global level but could be 
> overridden for a single TU). Do you have thoughts about that approach?

This would allow to specify whether to store each individual preamble in 
memory, which is more specific than necessary for my use case. This would shift 
the burden of passing around the `StorePreamblesInMemory` value to libclang 
users. Certainly more difficult to implement both in libclang and in KDevelop.

Advantages of getting rid of the option state setters entirely and passing 
options to per-TU APIs:

1. More flexibility (per-TU option specification).
2. Possibly more efficient (no need for atomics and mutexes).
3. Clearer to API users which TUs the modified options apply to and which TUs 
(created earlier) keep the original options.
4. Less risk of inconsistencies and bugs in libclang. Such as somehow passing a 
new option value to an old TU with unpredictable consequences.

Do the listed advantages explain your preference?

I am not sure what I would prefer from a hypothetical standpoint of a libclang 
maintainer. And you definitely have more experience in this area. So I won't 
argue for the index option state setters.

I'll probably implement the uncontroversial `CXIndexOptions` member first. And 
then, if I think implementing it is worth the effort, continue discussing 
`clang_parseTranslationUnitWithOptions()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

I am implementing the `StorePreamblesInMemory` bool option discussed earlier.  
It would be nice to be able to modify it at any time, because it can be an 
option in an IDE's UI and requiring to restart an IDE for the option change to 
take effect is undesirable. In order to make the setter thread-safe, the option 
can be stored as `std::atomic StorePreamblesInMemory` in `class CIndexer` 
and stored/loaded with `memory_order_relaxed`. Setting this option would apply 
only to subsequent `clang_parseTranslationUnit_Impl()` calls. The option can 
also be added to `CXIndexOptions` in order to allow setting its initial value 
reliably (not worrying whether it could be used before the setter gets called 
after index construction).

Is adding both the setter and the `CXIndexOptions` member OK or would you 
prefer only one of these two?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Thank you for the quick build fix. KDevelop's CMakeLists.txt disables this 
warning by adding the `-Wno-missing-field-initializers` flag. That's why I 
haven't noticed the warning while building KDevelop. Either I missed the 
warning while building LLVM or it is also disabled in a default GNU/Linux 
x86_64 build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

A clang-ppc64le-rhel build 
 failed:

  54.897 [28/169/148] Building CXX object 
tools/clang/unittests/libclang/CMakeFiles/libclangTests.dir/LibclangTest.cpp.o
  FAILED: 
tools/clang/unittests/libclang/CMakeFiles/libclangTests.dir/LibclangTest.cpp.o 
  /home/buildbots/clang.15.0.4/bin/clang++ --gcc-toolchain=/usr 
-DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE 
-D_LIBCPP_ENABLE_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS 
-D__STDC_LIMIT_MACROS 
-I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/unittests/libclang
 
-I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang/unittests/libclang
 
-I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang/include
 
-I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/include
 
-I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/include
 
-I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include
 
-I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/third-party/unittest/googletest/include
 
-I/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/third-party/unittest/googlemock/include
 -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror 
-Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion 
-Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color 
-ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual 
-Wno-nested-anon-types -O3 -DNDEBUG  -Wno-variadic-macros 
-Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -funwind-tables 
-fno-rtti -UNDEBUG -Wno-suggest-override -std=c++17 -MD -MT 
tools/clang/unittests/libclang/CMakeFiles/libclangTests.dir/LibclangTest.cpp.o 
-MF 
tools/clang/unittests/libclang/CMakeFiles/libclangTests.dir/LibclangTest.cpp.o.d
 -o 
tools/clang/unittests/libclang/CMakeFiles/libclangTests.dir/LibclangTest.cpp.o 
-c 
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang/unittests/libclang/LibclangTest.cpp
  
/home/buildbots/docker-RHEL84-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang/unittests/libclang/LibclangTest.cpp:488:50:
 error: missing field 'ThreadBackgroundPriorityForIndexing' initializer 
[-Werror,-Wmissing-field-initializers]
  CXIndexOptions Opts = {sizeof(CXIndexOptions)};
   ^
  1 error generated.

Same on ppc64le-lld-multistage-test 
.

Do you know why partial struct initialization is unsupported on this platform?
A simple workaround is to use `memset` in this single place. I am preparing a 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:3552
+ * If the cursor does not reference a bit field declaration or if the bit
+ * field's width does not depend on template parameters, 0 is returned.
+ */

vedgy wrote:
> collinbaker wrote:
> > vedgy wrote:
> > > I just thought how the new API could be used in KDevelop. Currently when 
> > > `clang_getFieldDeclBitWidth()` is positive, e.g. 2, KDevelop shows  ` : 
> > > 2` after the data member name in a tooltip. Ideally a 
> > > template-param-dependent expression (actual code) would be displayed 
> > > after the colon. If that's difficult to implement, `: [tparam-dependent]` 
> > > or `: ?` could be displayed instead. But it would be more convenient and 
> > > efficient to get this information by a single call to 
> > > `clang_getFieldDeclBitWidth()` instead of calling 
> > > `clang_isFieldDeclBitWidthDependent()` each time 
> > > `clang_getFieldDeclBitWidth()` returns `-1`. So how about returning `-2` 
> > > or `0` from `clang_getFieldDeclBitWidth()` instead of adding this new API?
> > I understand the motivation but I don't think requiring an extra call is 
> > asking too much of libclang clients, and it's one extra call that doesn't 
> > do much work and will be called rarely so I don't see efficiency concerns. 
> > Without strong reasons otherwise I think it's better to be explicit here.
> KDevelop calls `clang_getFieldDeclBitWidth()` for each encountered class 
> member declaration. `clang_isFieldDeclBitWidthDependent()` would have to be 
> called each time `clang_getFieldDeclBitWidth()` returns `-1`, which would be 
> most of the time, because few class members are bit-fields. The work this new 
> function does is the same as that of `clang_getFieldDeclBitWidth()`  
> (repeated).
> 
> If the concern about returning `-2` from `clang_getFieldDeclBitWidth()` is 
> cryptic return codes, an `enum` with named constants can be introduced.
> 
> If the concern is breaking backward compatibility for users that relied on 
> the returned value being positive or `-1`, then a replacement for 
> `clang_getFieldDeclBitWidth()` with the most convenient API should be 
> introduced and `clang_getFieldDeclBitWidth()` itself - deprecated.
> 
> KDevelop simply stores the value returned by `clang_getFieldDeclBitWidth()` 
> in an `int16_t m_bitWidth` data member and uses it later. So if `-2` is 
> returned, the only place in code to adjust would be the use of this data 
> member. With the current `clang_isFieldDeclBitWidthDependent()` 
> implementation, this function would have to be called, `-2` explicitly stored 
> in `m_bitWidth` and the use of `m_bitWidth` would have to be adjusted just 
> the same.
> 
> Have you considered potential usage of the added API in your project? Which 
> alternative would be more convenient to use?
One more API alternative is to replace `clang_isFieldDeclBitWidthDependent()` 
with `clang_isBitFieldDecl()`. The usage would be more straightforward and 
efficient: first call `clang_isBitFieldDecl()`; if it returns true (should be 
rare enough), call `clang_getFieldDeclBitWidth()`; if that returns `-1`, then 
the bit-field width must be unknown (dependent on template parameters). Such 
usage would still be less convenient and efficient compared to 
`clang_getFieldDeclBitWidth()` returning `-2` though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4172587 , @aaron.ballman 
wrote:

> Thank you, this LGTM! I have to head out shortly, so I'll land this on your 
> behalf tomorrow when I have the time to babysit the postcommit build farm. 
> However, if you'd like to request commit access for yourself, I think that'd 
> be reasonable as well: 
> https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access Let me 
> know which route you'd prefer going.

https://llvm.org/docs/Phabricator.html#committing-a-change says:

> Using the Arcanist tool can simplify the process of committing reviewed code 
> as it will retrieve reviewers, the Differential Revision, etc from the review 
> and place it in the commit message. You may also commit an accepted change 
> directly using git push, per the section in the getting started guide.

But how to use the Arcanist tool to push reviewed changes is not elaborated. As 
far as I can tell from the Arcanist documentation, if I have the commit in my 
local //main// branch which is currently checked out, I simply need to run `arc 
land` without arguments. Hopefully the Git pre-push hook 
 I have set up will 
run in this case.

I have no idea how to babysit the postcommit build farm, and so wouldn't commit 
when you don't have time anyway. I plan to make only one more change to 
libclang in the foreseeable future, so not sure learning to handle postcommit 
issues is justified. I'll leave this to your discretion as I have no idea how 
difficult and time-consuming this work is, compared to learning how to do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:3552
+ * If the cursor does not reference a bit field declaration or if the bit
+ * field's width does not depend on template parameters, 0 is returned.
+ */

collinbaker wrote:
> vedgy wrote:
> > I just thought how the new API could be used in KDevelop. Currently when 
> > `clang_getFieldDeclBitWidth()` is positive, e.g. 2, KDevelop shows  ` : 2` 
> > after the data member name in a tooltip. Ideally a template-param-dependent 
> > expression (actual code) would be displayed after the colon. If that's 
> > difficult to implement, `: [tparam-dependent]` or `: ?` could be displayed 
> > instead. But it would be more convenient and efficient to get this 
> > information by a single call to `clang_getFieldDeclBitWidth()` instead of 
> > calling `clang_isFieldDeclBitWidthDependent()` each time 
> > `clang_getFieldDeclBitWidth()` returns `-1`. So how about returning `-2` or 
> > `0` from `clang_getFieldDeclBitWidth()` instead of adding this new API?
> I understand the motivation but I don't think requiring an extra call is 
> asking too much of libclang clients, and it's one extra call that doesn't do 
> much work and will be called rarely so I don't see efficiency concerns. 
> Without strong reasons otherwise I think it's better to be explicit here.
KDevelop calls `clang_getFieldDeclBitWidth()` for each encountered class member 
declaration. `clang_isFieldDeclBitWidthDependent()` would have to be called 
each time `clang_getFieldDeclBitWidth()` returns `-1`, which would be most of 
the time, because few class members are bit-fields. The work this new function 
does is the same as that of `clang_getFieldDeclBitWidth()`  (repeated).

If the concern about returning `-2` from `clang_getFieldDeclBitWidth()` is 
cryptic return codes, an `enum` with named constants can be introduced.

If the concern is breaking backward compatibility for users that relied on the 
returned value being positive or `-1`, then a replacement for 
`clang_getFieldDeclBitWidth()` with the most convenient API should be 
introduced and `clang_getFieldDeclBitWidth()` itself - deprecated.

KDevelop simply stores the value returned by `clang_getFieldDeclBitWidth()` in 
an `int16_t m_bitWidth` data member and uses it later. So if `-2` is returned, 
the only place in code to adjust would be the use of this data member. With the 
current `clang_isFieldDeclBitWidthDependent()` implementation, this function 
would have to be called, `-2` explicitly stored in `m_bitWidth` and the use of 
`m_bitWidth` would have to be adjusted just the same.

Have you considered potential usage of the added API in your project? Which 
alternative would be more convenient to use?



Comment at: clang/tools/libclang/CXType.cpp:13
 
+#include "CXType.h"
 #include "CIndexer.h"

I guess //clang-format// did this include reordering. But it certainly looks 
out of place and the include order becomes wrong. So I think it should be 
reverted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy updated this revision to Diff 502632.
vedgy edited the summary of this revision.
vedgy added a comment.

Clean rebase on main branch where the base revision D143415 
 has already landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/tools/c-index-test/c-index-test.c
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CIndexer.h
  clang/tools/libclang/libclang.map
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/libclang/LibclangTest.cpp
  clang/unittests/libclang/TestUtils.h

Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -22,11 +22,11 @@
 #include 
 
 class LibclangParseTest : public ::testing::Test {
-  // std::greater<> to remove files before their parent dirs in TearDown().
-  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> FilesAndDirsToRemove;
   std::string TestDir;
   bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
@@ -40,7 +40,7 @@
 TestDir = std::string(Dir.str());
 TUFlags = CXTranslationUnit_DetailedPreprocessingRecord |
   clang_defaultEditingTranslationUnitOptions();
-Index = clang_createIndex(0, 0);
+CreateIndex();
 ClangTU = nullptr;
   }
   void TearDown() override {
@@ -48,7 +48,7 @@
 clang_disposeIndex(Index);
 
 namespace fs = llvm::sys::fs;
-for (const std::string  : Files)
+for (const std::string  : FilesAndDirsToRemove)
   EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
 if (RemoveTestDirRecursivelyDuringTeardown)
   EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
@@ -63,7 +63,7 @@
FileI != FileEnd; ++FileI) {
 ASSERT_NE(*FileI, ".");
 path::append(Path, *FileI);
-Files.emplace(Path.str());
+FilesAndDirsToRemove.emplace(Path.str());
   }
   Filename = std::string(Path.str());
 }
@@ -101,6 +101,9 @@
 return string;
   };
 
+protected:
+  virtual void CreateIndex() { Index = clang_createIndex(0, 0); }
+
 private:
   template
   static CXChildVisitResult TraverseStateless(CXCursor cx, CXCursor parent,
Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "TestUtils.h"
 #include "clang-c/Index.h"
 #include "clang-c/Rewrite.h"
 #include "llvm/ADT/StringRef.h"
@@ -14,7 +15,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
-#include "TestUtils.h"
+#include 
 #include 
 #include 
 #include 
@@ -355,6 +356,168 @@
   clang_ModuleMapDescriptor_dispose(MMD);
 }
 
+TEST_F(LibclangParseTest, GlobalOptions) {
+  EXPECT_EQ(clang_CXIndex_getGlobalOptions(Index), CXGlobalOpt_None);
+}
+
+class LibclangIndexOptionsTest : public LibclangParseTest {
+  virtual void AdjustOptions(CXIndexOptions ) {}
+
+protected:
+  void CreateIndex() override {
+CXIndexOptions Opts;
+memset(, 0, sizeof(Opts));
+Opts.Size = sizeof(CXIndexOptions);
+AdjustOptions(Opts);
+Index = clang_createIndexWithOptions();
+ASSERT_TRUE(Index);
+  }
+};
+
+TEST_F(LibclangIndexOptionsTest, GlobalOptions) {
+  EXPECT_EQ(clang_CXIndex_getGlobalOptions(Index), CXGlobalOpt_None);
+}
+
+class LibclangIndexingEnabledIndexOptionsTest
+: public LibclangIndexOptionsTest {
+  void AdjustOptions(CXIndexOptions ) override {
+Opts.ThreadBackgroundPriorityForIndexing = CXChoice_Enabled;
+  }
+};
+
+TEST_F(LibclangIndexingEnabledIndexOptionsTest, GlobalOptions) {
+  EXPECT_EQ(clang_CXIndex_getGlobalOptions(Index),
+CXGlobalOpt_ThreadBackgroundPriorityForIndexing);
+}
+
+class LibclangIndexingDisabledEditingEnabledIndexOptionsTest
+: public LibclangIndexOptionsTest {
+  void AdjustOptions(CXIndexOptions ) override {
+Opts.ThreadBackgroundPriorityForIndexing = CXChoice_Disabled;
+Opts.ThreadBackgroundPriorityForEditing = CXChoice_Enabled;
+  }
+};
+
+TEST_F(LibclangIndexingDisabledEditingEnabledIndexOptionsTest, GlobalOptions) {
+  EXPECT_EQ(clang_CXIndex_getGlobalOptions(Index),
+CXGlobalOpt_ThreadBackgroundPriorityForEditing);
+}
+
+class LibclangBothEnabledIndexOptionsTest : 

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:3552
+ * If the cursor does not reference a bit field declaration or if the bit
+ * field's width does not depend on template parameters, 0 is returned.
+ */

I just thought how the new API could be used in KDevelop. Currently when 
`clang_getFieldDeclBitWidth()` is positive, e.g. 2, KDevelop shows  ` : 2` 
after the data member name in a tooltip. Ideally a template-param-dependent 
expression (actual code) would be displayed after the colon. If that's 
difficult to implement, `: [tparam-dependent]` or `: ?` could be displayed 
instead. But it would be more convenient and efficient to get this information 
by a single call to `clang_getFieldDeclBitWidth()` instead of calling 
`clang_isFieldDeclBitWidthDependent()` each time `clang_getFieldDeclBitWidth()` 
returns `-1`. So how about returning `-2` or `0` from 
`clang_getFieldDeclBitWidth()` instead of adding this new API?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:3560
+ * If a cursor that is not a bit field declaration is passed in, or if the bit
+ * field's width expression cannot be evaluated, -1 is returned.
  */

Append the following line to the commit message to properly reference and 
(hopefully) close the bug automatically:
```
Fixes: https://github.com/llvm/llvm-project/issues/56644
```



Comment at: clang/tools/libclang/CXType.cpp:18
 #include "CXType.h"
+#include "clang-c/Index.h"
 #include "clang/AST/Decl.h"

Since the code worked fine before, I don't think this include should be added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added subscribers: aaron.ballman, vedgy.
vedgy added a comment.

I just verified that this patch fixes a KDevelop crash 
 reported by several users. Thank 
you!

@aaron.ballman, could you please review this fix?




Comment at: clang/include/clang-c/Index.h:3554
+ */
+CINDEX_LINKAGE unsigned clang_isFieldDeclBitWidthDependent(CXCursor C);
+

New API has to be mentioned in the //libclang// section of 
//clang/docs/ReleaseNotes.rst// and in the //LLVM_17// block of 
//clang/tools/libclang/libclang.map//.



Comment at: clang/tools/libclang/CXType.cpp:404
+  if (FD->isBitField() && !FD->getBitWidth()->isValueDependent())
 return FD->getBitWidthValue(getCursorContext(C));
 }

I thought of the same fix while analyzing the assertion failure here: 
https://bugs.kde.org/show_bug.cgi?id=438249#c19

An alternative implementation is to place this same check in 
`clang::FieldDecl::getBitWidthValue()`. This looks like a general issue that 
could affect non-libclang users of `clang::FieldDecl::getBitWidthValue()`. But 
apparently no one else has stumbled upon it.

`clang::FieldDecl::getBitWidthValue()` returns `unsigned` and has no 
error-reporting mechanism yet. It could return 
`std::numeric_limits::max()` (or `0` if that's an invalid bit width 
value) in case of the value dependence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130303

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy updated this revision to Diff 502376.
vedgy added a comment.

Replace `clang_getDefaultGlobalOptions()` with `CXChoice` as discussed.
A few unrelated small improvements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/tools/c-index-test/c-index-test.c
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CIndexer.h
  clang/tools/libclang/libclang.map
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/libclang/LibclangTest.cpp
  clang/unittests/libclang/TestUtils.h

Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -22,11 +22,11 @@
 #include 
 
 class LibclangParseTest : public ::testing::Test {
-  // std::greater<> to remove files before their parent dirs in TearDown().
-  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> FilesAndDirsToRemove;
   std::string TestDir;
   bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
@@ -40,7 +40,7 @@
 TestDir = std::string(Dir.str());
 TUFlags = CXTranslationUnit_DetailedPreprocessingRecord |
   clang_defaultEditingTranslationUnitOptions();
-Index = clang_createIndex(0, 0);
+CreateIndex();
 ClangTU = nullptr;
   }
   void TearDown() override {
@@ -48,7 +48,7 @@
 clang_disposeIndex(Index);
 
 namespace fs = llvm::sys::fs;
-for (const std::string  : Files)
+for (const std::string  : FilesAndDirsToRemove)
   EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
 if (RemoveTestDirRecursivelyDuringTeardown)
   EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
@@ -63,7 +63,7 @@
FileI != FileEnd; ++FileI) {
 ASSERT_NE(*FileI, ".");
 path::append(Path, *FileI);
-Files.emplace(Path.str());
+FilesAndDirsToRemove.emplace(Path.str());
   }
   Filename = std::string(Path.str());
 }
@@ -101,6 +101,9 @@
 return string;
   };
 
+protected:
+  virtual void CreateIndex() { Index = clang_createIndex(0, 0); }
+
 private:
   template
   static CXChildVisitResult TraverseStateless(CXCursor cx, CXCursor parent,
Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "TestUtils.h"
 #include "clang-c/Index.h"
 #include "clang-c/Rewrite.h"
 #include "llvm/ADT/StringRef.h"
@@ -14,7 +15,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
-#include "TestUtils.h"
+#include 
 #include 
 #include 
 #include 
@@ -355,6 +356,168 @@
   clang_ModuleMapDescriptor_dispose(MMD);
 }
 
+TEST_F(LibclangParseTest, GlobalOptions) {
+  EXPECT_EQ(clang_CXIndex_getGlobalOptions(Index), CXGlobalOpt_None);
+}
+
+class LibclangIndexOptionsTest : public LibclangParseTest {
+  virtual void AdjustOptions(CXIndexOptions ) {}
+
+protected:
+  void CreateIndex() override {
+CXIndexOptions Opts;
+memset(, 0, sizeof(Opts));
+Opts.Size = sizeof(CXIndexOptions);
+AdjustOptions(Opts);
+Index = clang_createIndexWithOptions();
+ASSERT_TRUE(Index);
+  }
+};
+
+TEST_F(LibclangIndexOptionsTest, GlobalOptions) {
+  EXPECT_EQ(clang_CXIndex_getGlobalOptions(Index), CXGlobalOpt_None);
+}
+
+class LibclangIndexingEnabledIndexOptionsTest
+: public LibclangIndexOptionsTest {
+  void AdjustOptions(CXIndexOptions ) override {
+Opts.ThreadBackgroundPriorityForIndexing = CXChoice_Enabled;
+  }
+};
+
+TEST_F(LibclangIndexingEnabledIndexOptionsTest, GlobalOptions) {
+  EXPECT_EQ(clang_CXIndex_getGlobalOptions(Index),
+CXGlobalOpt_ThreadBackgroundPriorityForIndexing);
+}
+
+class LibclangIndexingDisabledEditingEnabledIndexOptionsTest
+: public LibclangIndexOptionsTest {
+  void AdjustOptions(CXIndexOptions ) override {
+Opts.ThreadBackgroundPriorityForIndexing = CXChoice_Disabled;
+Opts.ThreadBackgroundPriorityForEditing = CXChoice_Enabled;
+  }
+};
+
+TEST_F(LibclangIndexingDisabledEditingEnabledIndexOptionsTest, GlobalOptions) {
+  EXPECT_EQ(clang_CXIndex_getGlobalOptions(Index),
+CXGlobalOpt_ThreadBackgroundPriorityForEditing);
+}
+
+class LibclangBothEnabledIndexOptionsTest : public LibclangIndexOptionsTest {
+  void 

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:329
+ * CXIndexOptions Opts = { sizeof(CXIndexOptions),
+ * clang_getDefaultGlobalOptions() };
+ * \endcode

aaron.ballman wrote:
> vedgy wrote:
> > vedgy wrote:
> > > aaron.ballman wrote:
> > > > vedgy wrote:
> > > > > When I almost finished the requested changes, I remembered that the 
> > > > > return value of `clang_getDefaultGlobalOptions()` depends on 
> > > > > environment variables, and thus `0` is not necessarily the default. 
> > > > > Adjusted the changes and updated this revision.
> > > > > 
> > > > > Does the extra requirement to non-zero initialize this second member 
> > > > > sway your opinion on the usefulness of the helper function `inline 
> > > > > CXIndexOptions clang_getDefaultIndexOptions()`? Note that there may 
> > > > > be same (environment) or other important reasons why future new 
> > > > > options couldn't be zeroes by default.
> > > > Thinking out loud a bit... (potentially bad idea incoming)
> > > > 
> > > > What if we dropped `clang_getDefaultGlobalOptions()` and instead made a 
> > > > change to `CXGlobalOptFlags`:
> > > > ```
> > > > typedef enum {
> > > >   /**
> > > >* Used to indicate that the default CXIndex options are used. By 
> > > > default, no
> > > >* global options will be used. However, environment variables may 
> > > > change which
> > > >* global options are in effect at runtime.
> > > >*/
> > > >   CXGlobalOpt_Default = 0x0,
> > > > 
> > > >   /**
> > > >* Used to indicate that threads that libclang creates for indexing
> > > >* purposes should use background priority.
> > > >*
> > > >* Affects #clang_indexSourceFile, #clang_indexTranslationUnit,
> > > >* #clang_parseTranslationUnit, #clang_saveTranslationUnit.
> > > >*/
> > > >   CXGlobalOpt_ThreadBackgroundPriorityForIndexing = 0x1,
> > > > 
> > > >   /**
> > > >* Used to indicate that threads that libclang creates for editing
> > > >* purposes should use background priority.
> > > >*
> > > >* Affects #clang_reparseTranslationUnit, #clang_codeCompleteAt,
> > > >* #clang_annotateTokens
> > > >*/
> > > >   CXGlobalOpt_ThreadBackgroundPriorityForEditing = 0x2,
> > > > 
> > > >   /**
> > > >* Used to indicate that all threads that libclang creates should use
> > > >* background priority.
> > > >*/
> > > >   CXGlobalOpt_ThreadBackgroundPriorityForAll =
> > > >   CXGlobalOpt_ThreadBackgroundPriorityForIndexing |
> > > >   CXGlobalOpt_ThreadBackgroundPriorityForEditing,
> > > > 
> > > >   /**
> > > >* Used to indicate that no global options should be used, even
> > > >* in the presence of environment variables.
> > > >*/
> > > >   CXGlobalOpt_None = 0x
> > > > } CXGlobalOptFlags;
> > > > ```
> > > > so that when the user passes `0` they get the previous behavior.
> > > > 
> > > > `clang_CXIndex_setGlobalOptions()` would remain deprecated. 
> > > > `clang_CXIndex_getGlobalOptions()` would be interesting though -- would 
> > > > it return `CXGlobalOpt_None` or `CXGlobalOpt_Default` in the event the 
> > > > index was created without any global options? Hmmm.
> > > > 
> > > > Err, actually, I suppose this won't work too well because then code 
> > > > silently changes behavior if it does 
> > > > `clang_CXIndex_setGlobalOptions(CXGlobalOpt_None);` because that would 
> > > > change from "do what the environment says" to "ignore the environment". 
> > > > But I have to wonder whether anyone actually *does* that or not... my 
> > > > intuition is that folks would not call 
> > > > `clang_CXIndex_setGlobalOptions()` at all unless they were setting an 
> > > > option to a non-none value. We could rename `CXGlobalOpt_None` to 
> > > > `CXGlobalOpt_Nothing` (or something along those lines) to force a 
> > > > compilation error, but that's a bit of a nuclear option for what's 
> > > > supposed to be a stable API.
> > > > 
> > > > So I'm on the fence, I guess. I'd still prefer for zero to give 
> > > > sensible defaults and I don't think there's enough use of the global 
> > > > options + environment variables to matter. But I also don't like 
> > > > silently breaking code, so my idea above may be a nonstarter.
> > > > 
> > > > I suppose another possible idea is: deprecate the notion of global 
> > > > options enum and setter/getter entirely, add two new fields to 
> > > > `CXIndexOptions`:
> > > > ```
> > > > typedef enum {
> > > >   CXChoice_Default = 0,
> > > >   CXChoice_Enabled = 1,
> > > >   CXChoice_Disabled = 2
> > > > } CXChoice;
> > > > 
> > > > ...
> > > > unsigned ThreadPriorityBackgroundForIndexing;
> > > > unsigned ThreadPriorityBackgroundForEditing;
> > > > ...
> > > > ```
> > > > so that `0` gives the correct default behavior based on environment 
> > > > variable. There would be no global setter or getter for this 
> > > > information (and we'd eventually remove 
> > > > 

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy marked an inline comment as not done.
vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:329
+ * CXIndexOptions Opts = { sizeof(CXIndexOptions),
+ * clang_getDefaultGlobalOptions() };
+ * \endcode

vedgy wrote:
> aaron.ballman wrote:
> > vedgy wrote:
> > > When I almost finished the requested changes, I remembered that the 
> > > return value of `clang_getDefaultGlobalOptions()` depends on environment 
> > > variables, and thus `0` is not necessarily the default. Adjusted the 
> > > changes and updated this revision.
> > > 
> > > Does the extra requirement to non-zero initialize this second member sway 
> > > your opinion on the usefulness of the helper function `inline 
> > > CXIndexOptions clang_getDefaultIndexOptions()`? Note that there may be 
> > > same (environment) or other important reasons why future new options 
> > > couldn't be zeroes by default.
> > Thinking out loud a bit... (potentially bad idea incoming)
> > 
> > What if we dropped `clang_getDefaultGlobalOptions()` and instead made a 
> > change to `CXGlobalOptFlags`:
> > ```
> > typedef enum {
> >   /**
> >* Used to indicate that the default CXIndex options are used. By 
> > default, no
> >* global options will be used. However, environment variables may change 
> > which
> >* global options are in effect at runtime.
> >*/
> >   CXGlobalOpt_Default = 0x0,
> > 
> >   /**
> >* Used to indicate that threads that libclang creates for indexing
> >* purposes should use background priority.
> >*
> >* Affects #clang_indexSourceFile, #clang_indexTranslationUnit,
> >* #clang_parseTranslationUnit, #clang_saveTranslationUnit.
> >*/
> >   CXGlobalOpt_ThreadBackgroundPriorityForIndexing = 0x1,
> > 
> >   /**
> >* Used to indicate that threads that libclang creates for editing
> >* purposes should use background priority.
> >*
> >* Affects #clang_reparseTranslationUnit, #clang_codeCompleteAt,
> >* #clang_annotateTokens
> >*/
> >   CXGlobalOpt_ThreadBackgroundPriorityForEditing = 0x2,
> > 
> >   /**
> >* Used to indicate that all threads that libclang creates should use
> >* background priority.
> >*/
> >   CXGlobalOpt_ThreadBackgroundPriorityForAll =
> >   CXGlobalOpt_ThreadBackgroundPriorityForIndexing |
> >   CXGlobalOpt_ThreadBackgroundPriorityForEditing,
> > 
> >   /**
> >* Used to indicate that no global options should be used, even
> >* in the presence of environment variables.
> >*/
> >   CXGlobalOpt_None = 0x
> > } CXGlobalOptFlags;
> > ```
> > so that when the user passes `0` they get the previous behavior.
> > 
> > `clang_CXIndex_setGlobalOptions()` would remain deprecated. 
> > `clang_CXIndex_getGlobalOptions()` would be interesting though -- would it 
> > return `CXGlobalOpt_None` or `CXGlobalOpt_Default` in the event the index 
> > was created without any global options? Hmmm.
> > 
> > Err, actually, I suppose this won't work too well because then code 
> > silently changes behavior if it does 
> > `clang_CXIndex_setGlobalOptions(CXGlobalOpt_None);` because that would 
> > change from "do what the environment says" to "ignore the environment". But 
> > I have to wonder whether anyone actually *does* that or not... my intuition 
> > is that folks would not call `clang_CXIndex_setGlobalOptions()` at all 
> > unless they were setting an option to a non-none value. We could rename 
> > `CXGlobalOpt_None` to `CXGlobalOpt_Nothing` (or something along those 
> > lines) to force a compilation error, but that's a bit of a nuclear option 
> > for what's supposed to be a stable API.
> > 
> > So I'm on the fence, I guess. I'd still prefer for zero to give sensible 
> > defaults and I don't think there's enough use of the global options + 
> > environment variables to matter. But I also don't like silently breaking 
> > code, so my idea above may be a nonstarter.
> > 
> > I suppose another possible idea is: deprecate the notion of global options 
> > enum and setter/getter entirely, add two new fields to `CXIndexOptions`:
> > ```
> > typedef enum {
> >   CXChoice_Default = 0,
> >   CXChoice_Enabled = 1,
> >   CXChoice_Disabled = 2
> > } CXChoice;
> > 
> > ...
> > unsigned ThreadPriorityBackgroundForIndexing;
> > unsigned ThreadPriorityBackgroundForEditing;
> > ...
> > ```
> > so that `0` gives the correct default behavior based on environment 
> > variable. There would be no global setter or getter for this information 
> > (and we'd eventually remove `clang_CXIndex_[gs]etGlobalOptions()`).
> > I suppose this won't work too well because then code silently changes 
> > behavior if it does `clang_CXIndex_setGlobalOptions(CXGlobalOpt_None);` 
> > because that would change from "do what the environment says" to "ignore 
> > the environment".
> No, the current consequence of such a call already is to ignore the 
> environment. What would change is the consequence of 

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:329
+ * CXIndexOptions Opts = { sizeof(CXIndexOptions),
+ * clang_getDefaultGlobalOptions() };
+ * \endcode

aaron.ballman wrote:
> vedgy wrote:
> > When I almost finished the requested changes, I remembered that the return 
> > value of `clang_getDefaultGlobalOptions()` depends on environment 
> > variables, and thus `0` is not necessarily the default. Adjusted the 
> > changes and updated this revision.
> > 
> > Does the extra requirement to non-zero initialize this second member sway 
> > your opinion on the usefulness of the helper function `inline 
> > CXIndexOptions clang_getDefaultIndexOptions()`? Note that there may be same 
> > (environment) or other important reasons why future new options couldn't be 
> > zeroes by default.
> Thinking out loud a bit... (potentially bad idea incoming)
> 
> What if we dropped `clang_getDefaultGlobalOptions()` and instead made a 
> change to `CXGlobalOptFlags`:
> ```
> typedef enum {
>   /**
>* Used to indicate that the default CXIndex options are used. By default, 
> no
>* global options will be used. However, environment variables may change 
> which
>* global options are in effect at runtime.
>*/
>   CXGlobalOpt_Default = 0x0,
> 
>   /**
>* Used to indicate that threads that libclang creates for indexing
>* purposes should use background priority.
>*
>* Affects #clang_indexSourceFile, #clang_indexTranslationUnit,
>* #clang_parseTranslationUnit, #clang_saveTranslationUnit.
>*/
>   CXGlobalOpt_ThreadBackgroundPriorityForIndexing = 0x1,
> 
>   /**
>* Used to indicate that threads that libclang creates for editing
>* purposes should use background priority.
>*
>* Affects #clang_reparseTranslationUnit, #clang_codeCompleteAt,
>* #clang_annotateTokens
>*/
>   CXGlobalOpt_ThreadBackgroundPriorityForEditing = 0x2,
> 
>   /**
>* Used to indicate that all threads that libclang creates should use
>* background priority.
>*/
>   CXGlobalOpt_ThreadBackgroundPriorityForAll =
>   CXGlobalOpt_ThreadBackgroundPriorityForIndexing |
>   CXGlobalOpt_ThreadBackgroundPriorityForEditing,
> 
>   /**
>* Used to indicate that no global options should be used, even
>* in the presence of environment variables.
>*/
>   CXGlobalOpt_None = 0x
> } CXGlobalOptFlags;
> ```
> so that when the user passes `0` they get the previous behavior.
> 
> `clang_CXIndex_setGlobalOptions()` would remain deprecated. 
> `clang_CXIndex_getGlobalOptions()` would be interesting though -- would it 
> return `CXGlobalOpt_None` or `CXGlobalOpt_Default` in the event the index was 
> created without any global options? Hmmm.
> 
> Err, actually, I suppose this won't work too well because then code silently 
> changes behavior if it does 
> `clang_CXIndex_setGlobalOptions(CXGlobalOpt_None);` because that would change 
> from "do what the environment says" to "ignore the environment". But I have 
> to wonder whether anyone actually *does* that or not... my intuition is that 
> folks would not call `clang_CXIndex_setGlobalOptions()` at all unless they 
> were setting an option to a non-none value. We could rename 
> `CXGlobalOpt_None` to `CXGlobalOpt_Nothing` (or something along those lines) 
> to force a compilation error, but that's a bit of a nuclear option for what's 
> supposed to be a stable API.
> 
> So I'm on the fence, I guess. I'd still prefer for zero to give sensible 
> defaults and I don't think there's enough use of the global options + 
> environment variables to matter. But I also don't like silently breaking 
> code, so my idea above may be a nonstarter.
> 
> I suppose another possible idea is: deprecate the notion of global options 
> enum and setter/getter entirely, add two new fields to `CXIndexOptions`:
> ```
> typedef enum {
>   CXChoice_Default = 0,
>   CXChoice_Enabled = 1,
>   CXChoice_Disabled = 2
> } CXChoice;
> 
> ...
> unsigned ThreadPriorityBackgroundForIndexing;
> unsigned ThreadPriorityBackgroundForEditing;
> ...
> ```
> so that `0` gives the correct default behavior based on environment variable. 
> There would be no global setter or getter for this information (and we'd 
> eventually remove `clang_CXIndex_[gs]etGlobalOptions()`).
> I suppose this won't work too well because then code silently changes 
> behavior if it does `clang_CXIndex_setGlobalOptions(CXGlobalOpt_None);` 
> because that would change from "do what the environment says" to "ignore the 
> environment".
No, the current consequence of such a call already is to ignore the 
environment. What would change is the consequence of calling 
`clang_CXIndex_setGlobalOptions(0);` - from "ignore the environment" to "do 
what the environment says".

> But I have to wonder whether anyone actually *does* that or not... my 
> intuition is that folks would not call `clang_CXIndex_setGlobalOptions()` at 
> all 

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy marked 2 inline comments as done.
vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:329
+ * CXIndexOptions Opts = { sizeof(CXIndexOptions),
+ * clang_getDefaultGlobalOptions() };
+ * \endcode

When I almost finished the requested changes, I remembered that the return 
value of `clang_getDefaultGlobalOptions()` depends on environment variables, 
and thus `0` is not necessarily the default. Adjusted the changes and updated 
this revision.

Does the extra requirement to non-zero initialize this second member sway your 
opinion on the usefulness of the helper function `inline CXIndexOptions 
clang_getDefaultIndexOptions()`? Note that there may be same (environment) or 
other important reasons why future new options couldn't be zeroes by default.



Comment at: clang/tools/c-index-test/c-index-test.c:79
+Opts.PreambleStoragePath = NULL;
+Opts.InvocationEmissionPath = 
getenv("CINDEXTEST_INVOCATION_EMISSION_PATH");
+

aaron.ballman wrote:
> vedgy wrote:
> > aaron.ballman wrote:
> > > vedgy wrote:
> > > > aaron.ballman wrote:
> > > > > vedgy wrote:
> > > > > > When a libclang user needs to override a single option in 
> > > > > > `CXIndexOptions`, [s]he has to set every member of the struct 
> > > > > > explicitly. When new options are added, each libclang user needs to 
> > > > > > update the code that sets the options under `CINDEX_VERSION_MINOR` 
> > > > > > `#if`s. Accidentally omitting even one member assignment risks 
> > > > > > undefined or wrong behavior. How about adding an `inline` helper 
> > > > > > function `CXIndexOptions clang_getDefaultIndexOptions()`, which 
> > > > > > assigns default values to all struct members? Libclang users can 
> > > > > > then call this function to obtain the default configuration, then 
> > > > > > tweak only the members they want to override.
> > > > > > 
> > > > > > If this suggestion is to be implemented, how to deal with 
> > > > > > [[https://stackoverflow.com/questions/68004269/differences-of-the-inline-keyword-in-c-and-c|the
> > > > > >  differences of the inline keyword in C and C++]]?
> > > > > By default, `0` should give you the most reasonable default behavior 
> > > > > for most of the existing options (and new options should follow the 
> > > > > same pattern). Ideally, users should be able to do:
> > > > > ```
> > > > > CXIndexOptions Opts;
> > > > > memset(, 0, sizeof(Opts));
> > > > > Opts.Size = sizeof(Opts);
> > > > > Opts.Whatever = 12;
> > > > > CXIndex Idx = clang_createIndexWithOptions();
> > > > > ```
> > > > > Global options defaulting to 0 is fine (uses regular thread 
> > > > > priorities), we don't think want to default to excluding declarations 
> > > > > from PCH, and we want to use the default preamble and invocation 
> > > > > emission paths (if any). The only option that nonzero as a default 
> > > > > *might* make sense for is displaying diagnostics, but even that seems 
> > > > > reasonable to expect the developer to manually enable.
> > > > > 
> > > > > So I don't know that we need a function to get us default indexing 
> > > > > options as `0` should be a reasonable default for all of the options. 
> > > > > As we add new options, we need to be careful to add them in backwards 
> > > > > compatible ways where `0` means "do the most likely thing".
> > > > > 
> > > > > WDYT?
> > > > The disadvantages of committing to defaulting to `0`:
> > > > 1. The usage you propose is still more verbose and error-prone than
> > > > ```
> > > > CXIndexOptions Opts = clang_getDefaultIndexOptions();
> > > > Opts.Whatever = 12;
> > > > CXIndex Idx = clang_createIndexWithOptions();
> > > > ```
> > > > 2. The `memset` would look very unclean in modern C++ code.
> > > > 3. The `0` commitment may force unnatural naming of a future option to 
> > > > invert its meaning.
> > > > 
> > > > The advantages:
> > > > 1. No need to implement the inline function now.
> > > > 2. Faster, but I am sure this performance difference doesn't matter. 
> > > > Even a non-inline function call itself (even if it did nothing) to 
> > > > `clang_createIndexWithOptions()` should take longer than assigning a 
> > > > few values to built-in-typed members.
> > > > 
> > > > Another advantage of not having to remember to update the inline 
> > > > function's implementation when new options are added is counterbalanced 
> > > > by the need to be careful to add new options in backwards compatible 
> > > > way where `0` is the default.
> > > > 
> > > > Any other advantages of the `0` that I miss? Maybe there are some 
> > > > advantages for C users, but I suspect most libclang users are C++.
> > > >The usage you propose is still more verbose and error-prone than
> > > > ```
> > > > CXIndexOptions Opts = clang_getDefaultIndexOptions();
> > > > Opts.Whatever = 12;
> > > > CXIndex Idx = clang_createIndexWithOptions();
> > > > ```
> > > 
> > > I see it as being roughly 

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy updated this revision to Diff 501559.
vedgy added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/tools/c-index-test/c-index-test.c
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CIndexer.h
  clang/tools/libclang/libclang.map
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/libclang/LibclangTest.cpp
  clang/unittests/libclang/TestUtils.h

Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -22,11 +22,11 @@
 #include 
 
 class LibclangParseTest : public ::testing::Test {
-  // std::greater<> to remove files before their parent dirs in TearDown().
-  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> FilesAndDirsToRemove;
   std::string TestDir;
   bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
@@ -40,7 +40,7 @@
 TestDir = std::string(Dir.str());
 TUFlags = CXTranslationUnit_DetailedPreprocessingRecord |
   clang_defaultEditingTranslationUnitOptions();
-Index = clang_createIndex(0, 0);
+CreateIndex();
 ClangTU = nullptr;
   }
   void TearDown() override {
@@ -48,7 +48,7 @@
 clang_disposeIndex(Index);
 
 namespace fs = llvm::sys::fs;
-for (const std::string  : Files)
+for (const std::string  : FilesAndDirsToRemove)
   EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
 if (RemoveTestDirRecursivelyDuringTeardown)
   EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
@@ -63,7 +63,7 @@
FileI != FileEnd; ++FileI) {
 ASSERT_NE(*FileI, ".");
 path::append(Path, *FileI);
-Files.emplace(Path.str());
+FilesAndDirsToRemove.emplace(Path.str());
   }
   Filename = std::string(Path.str());
 }
@@ -101,6 +101,9 @@
 return string;
   };
 
+protected:
+  virtual void CreateIndex() { Index = clang_createIndex(0, 0); }
+
 private:
   template
   static CXChildVisitResult TraverseStateless(CXCursor cx, CXCursor parent,
Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "TestUtils.h"
 #include "clang-c/Index.h"
 #include "clang-c/Rewrite.h"
 #include "llvm/ADT/StringRef.h"
@@ -14,7 +15,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
-#include "TestUtils.h"
+#include 
 #include 
 #include 
 #include 
@@ -355,6 +356,110 @@
   clang_ModuleMapDescriptor_dispose(MMD);
 }
 
+class LibclangPreambleStorageTest : public LibclangParseTest {
+  std::string Main = "main.cpp";
+
+protected:
+  std::string PreambleDir;
+  void InitializePreambleDir() {
+llvm::SmallString<128> PathBuffer(TestDir);
+llvm::sys::path::append(PathBuffer, "preambles");
+namespace fs = llvm::sys::fs;
+ASSERT_FALSE(fs::create_directory(PathBuffer, false, fs::perms::owner_all));
+
+PreambleDir = static_cast(PathBuffer);
+FilesAndDirsToRemove.insert(PreambleDir);
+  }
+
+public:
+  void CountPreamblesInPreambleDir(int PreambleCount) {
+// For some reason, the preamble is not created without '\n' before `int`.
+WriteFile(Main, "\nint main() {}");
+
+TUFlags |= CXTranslationUnit_CreatePreambleOnFirstParse;
+ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0,
+ nullptr, 0, TUFlags);
+
+int FileCount = 0;
+
+namespace fs = llvm::sys::fs;
+std::error_code EC;
+for (fs::directory_iterator File(PreambleDir, EC), FileEnd;
+ File != FileEnd && !EC; File.increment(EC)) {
+  ++FileCount;
+
+  EXPECT_EQ(File->type(), fs::file_type::regular_file);
+
+  const auto Filename = llvm::sys::path::filename(File->path());
+  EXPECT_EQ(Filename.size(), std::strlen("preamble-%%.pch"));
+  EXPECT_TRUE(Filename.startswith("preamble-"));
+  EXPECT_TRUE(Filename.endswith(".pch"));
+
+  const auto Status = File->status();
+  ASSERT_TRUE(Status);
+  if (false) {
+// The permissions assertion below fails, because the .pch.tmp file is
+// created with default permissions and replaces the .pch file along
+// with 

[PATCH] D143415: LibclangTest: remove libclang-test-* tmp dir reliably

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143415#4161460 , @aaron.ballman 
wrote:

> In D143415#4160550 , @vedgy wrote:
>
>> Can someone merge/land this review? I don't have commit access.
>
> I'm happy to do that for you -- what name and email address would you like me 
> to use for patch attribution?

user.email=igor...@gmail.com
user.name=Igor Kushnir

A pity that Phabricator doesn't pick up git user name and email.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143415

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143415: LibclangTest: remove libclang-test-* tmp dir reliably

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Can someone merge/land this review? I don't have commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143415

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/tools/c-index-test/c-index-test.c:79
+Opts.PreambleStoragePath = NULL;
+Opts.InvocationEmissionPath = 
getenv("CINDEXTEST_INVOCATION_EMISSION_PATH");
+

aaron.ballman wrote:
> vedgy wrote:
> > aaron.ballman wrote:
> > > vedgy wrote:
> > > > When a libclang user needs to override a single option in 
> > > > `CXIndexOptions`, [s]he has to set every member of the struct 
> > > > explicitly. When new options are added, each libclang user needs to 
> > > > update the code that sets the options under `CINDEX_VERSION_MINOR` 
> > > > `#if`s. Accidentally omitting even one member assignment risks 
> > > > undefined or wrong behavior. How about adding an `inline` helper 
> > > > function `CXIndexOptions clang_getDefaultIndexOptions()`, which assigns 
> > > > default values to all struct members? Libclang users can then call this 
> > > > function to obtain the default configuration, then tweak only the 
> > > > members they want to override.
> > > > 
> > > > If this suggestion is to be implemented, how to deal with 
> > > > [[https://stackoverflow.com/questions/68004269/differences-of-the-inline-keyword-in-c-and-c|the
> > > >  differences of the inline keyword in C and C++]]?
> > > By default, `0` should give you the most reasonable default behavior for 
> > > most of the existing options (and new options should follow the same 
> > > pattern). Ideally, users should be able to do:
> > > ```
> > > CXIndexOptions Opts;
> > > memset(, 0, sizeof(Opts));
> > > Opts.Size = sizeof(Opts);
> > > Opts.Whatever = 12;
> > > CXIndex Idx = clang_createIndexWithOptions();
> > > ```
> > > Global options defaulting to 0 is fine (uses regular thread priorities), 
> > > we don't think want to default to excluding declarations from PCH, and we 
> > > want to use the default preamble and invocation emission paths (if any). 
> > > The only option that nonzero as a default *might* make sense for is 
> > > displaying diagnostics, but even that seems reasonable to expect the 
> > > developer to manually enable.
> > > 
> > > So I don't know that we need a function to get us default indexing 
> > > options as `0` should be a reasonable default for all of the options. As 
> > > we add new options, we need to be careful to add them in backwards 
> > > compatible ways where `0` means "do the most likely thing".
> > > 
> > > WDYT?
> > The disadvantages of committing to defaulting to `0`:
> > 1. The usage you propose is still more verbose and error-prone than
> > ```
> > CXIndexOptions Opts = clang_getDefaultIndexOptions();
> > Opts.Whatever = 12;
> > CXIndex Idx = clang_createIndexWithOptions();
> > ```
> > 2. The `memset` would look very unclean in modern C++ code.
> > 3. The `0` commitment may force unnatural naming of a future option to 
> > invert its meaning.
> > 
> > The advantages:
> > 1. No need to implement the inline function now.
> > 2. Faster, but I am sure this performance difference doesn't matter. Even a 
> > non-inline function call itself (even if it did nothing) to 
> > `clang_createIndexWithOptions()` should take longer than assigning a few 
> > values to built-in-typed members.
> > 
> > Another advantage of not having to remember to update the inline function's 
> > implementation when new options are added is counterbalanced by the need to 
> > be careful to add new options in backwards compatible way where `0` is the 
> > default.
> > 
> > Any other advantages of the `0` that I miss? Maybe there are some 
> > advantages for C users, but I suspect most libclang users are C++.
> >The usage you propose is still more verbose and error-prone than
> > ```
> > CXIndexOptions Opts = clang_getDefaultIndexOptions();
> > Opts.Whatever = 12;
> > CXIndex Idx = clang_createIndexWithOptions();
> > ```
> 
> I see it as being roughly the same amount of verbosity and proneness to 
> error, but maybe that's just me.
> 
> > The memset would look very unclean in modern C++ code.
> 
> I don't see this as a problem. 1) `std::memset` gets plenty of usage in 
> modern C++ still, 2) you can also initialize with ` = { 
> sizeof(CXIndexOptions) };` and rely on the zero init for subsequent members 
> after the first (personally, I think that's too clever, but reasonable people 
> may disagree), 3) this is a C API, folks using C++ may wish to wrap it in a 
> better interface for C++ anyway.
>  
> > The 0 commitment may force unnatural naming of a future option to invert 
> > its meaning.
> 
> I do worry about this one, especially given there's no way to predict what 
> future options we'll need. But it also forces us to think about what the 
> correct default behavior should be, whereas if we push it off to a helper 
> function, we make it harder for everyone who didn't know to use that helper 
> function for whatever reason.
> 
> > Any other advantages of the 0 that I miss? Maybe there are some advantages 
> > for C users, but I suspect most libclang users are 

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/tools/c-index-test/c-index-test.c:79
+Opts.PreambleStoragePath = NULL;
+Opts.InvocationEmissionPath = 
getenv("CINDEXTEST_INVOCATION_EMISSION_PATH");
+

aaron.ballman wrote:
> vedgy wrote:
> > When a libclang user needs to override a single option in `CXIndexOptions`, 
> > [s]he has to set every member of the struct explicitly. When new options 
> > are added, each libclang user needs to update the code that sets the 
> > options under `CINDEX_VERSION_MINOR` `#if`s. Accidentally omitting even one 
> > member assignment risks undefined or wrong behavior. How about adding an 
> > `inline` helper function `CXIndexOptions clang_getDefaultIndexOptions()`, 
> > which assigns default values to all struct members? Libclang users can then 
> > call this function to obtain the default configuration, then tweak only the 
> > members they want to override.
> > 
> > If this suggestion is to be implemented, how to deal with 
> > [[https://stackoverflow.com/questions/68004269/differences-of-the-inline-keyword-in-c-and-c|the
> >  differences of the inline keyword in C and C++]]?
> By default, `0` should give you the most reasonable default behavior for most 
> of the existing options (and new options should follow the same pattern). 
> Ideally, users should be able to do:
> ```
> CXIndexOptions Opts;
> memset(, 0, sizeof(Opts));
> Opts.Size = sizeof(Opts);
> Opts.Whatever = 12;
> CXIndex Idx = clang_createIndexWithOptions();
> ```
> Global options defaulting to 0 is fine (uses regular thread priorities), we 
> don't think want to default to excluding declarations from PCH, and we want 
> to use the default preamble and invocation emission paths (if any). The only 
> option that nonzero as a default *might* make sense for is displaying 
> diagnostics, but even that seems reasonable to expect the developer to 
> manually enable.
> 
> So I don't know that we need a function to get us default indexing options as 
> `0` should be a reasonable default for all of the options. As we add new 
> options, we need to be careful to add them in backwards compatible ways where 
> `0` means "do the most likely thing".
> 
> WDYT?
The disadvantages of committing to defaulting to `0`:
1. The usage you propose is still more verbose and error-prone than
```
CXIndexOptions Opts = clang_getDefaultIndexOptions();
Opts.Whatever = 12;
CXIndex Idx = clang_createIndexWithOptions();
```
2. The `memset` would look very unclean in modern C++ code.
3. The `0` commitment may force unnatural naming of a future option to invert 
its meaning.

The advantages:
1. No need to implement the inline function now.
2. Faster, but I am sure this performance difference doesn't matter. Even a 
non-inline function call itself (even if it did nothing) to 
`clang_createIndexWithOptions()` should take longer than assigning a few values 
to built-in-typed members.

Another advantage of not having to remember to update the inline function's 
implementation when new options are added is counterbalanced by the need to be 
careful to add new options in backwards compatible way where `0` is the default.

Any other advantages of the `0` that I miss? Maybe there are some advantages 
for C users, but I suspect most libclang users are C++.



Comment at: clang/tools/c-index-test/c-index-test.c:2079
+  if (!Idx)
+return -1;
 

aaron.ballman wrote:
> vedgy wrote:
> > Not sure `-1` is the right value to return here. This return value becomes 
> > the exit code of the `c-index-test` executable.
> I think `-1` is fine -- the important thing is a nonzero return code so it's 
> logged as an error rather than a valid result.
I have noticed that these functions sometimes return `-1` and sometimes `1` on 
different errors. I thought that perhaps there is some difference in these two 
return values, but I couldn't figure out what it might be. Since telling the 
difference is not straightforward, you are probably right that there is //no// 
meaningful difference.



Comment at: clang/tools/libclang/CIndex.cpp:3701-3702
+CXIndex clang_createIndexWithOptions(const CXIndexOptions *options) {
+  if (options->Size != sizeof(CXIndexOptions))
+return NULL;
+  CIndexer *CIdxr = clang_createIndex_Impl(options->ExcludeDeclarationsFromPCH,

aaron.ballman wrote:
> I think we want this to be `>` and not `!=`, maybe.
> 
> If the sizes are equal, we're on the happy path.
> 
> If the options from the caller are smaller than the options we know about, 
> that should be okay because we won't attempt read the options not provided 
> and instead rely on the default behavior being reasonable.
> 
> If the options from the caller are larger than the options we know about, we 
> have to assume the user set an option we can't handle, and thus failing the 
> request is reasonable.
> 
> So the way I'm envisioning us reading the options is:
> ```
> if 

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/include/clang-c/Index.h:319
+   */
+  size_t Size;
+  /**

The type is `size_t` instead of the agreed upon `unsigned`, because the 
addition of `unsigned GlobalOptions` below means that `unsigned Size` no longer 
reduces the overall struct's size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/tools/c-index-test/c-index-test.c:79
+Opts.PreambleStoragePath = NULL;
+Opts.InvocationEmissionPath = 
getenv("CINDEXTEST_INVOCATION_EMISSION_PATH");
+

When a libclang user needs to override a single option in `CXIndexOptions`, 
[s]he has to set every member of the struct explicitly. When new options are 
added, each libclang user needs to update the code that sets the options under 
`CINDEX_VERSION_MINOR` `#if`s. Accidentally omitting even one member assignment 
risks undefined or wrong behavior. How about adding an `inline` helper function 
`CXIndexOptions clang_getDefaultIndexOptions()`, which assigns default values 
to all struct members? Libclang users can then call this function to obtain the 
default configuration, then tweak only the members they want to override.

If this suggestion is to be implemented, how to deal with 
[[https://stackoverflow.com/questions/68004269/differences-of-the-inline-keyword-in-c-and-c|the
 differences of the inline keyword in C and C++]]?



Comment at: clang/tools/c-index-test/c-index-test.c:2079
+  if (!Idx)
+return -1;
 

Not sure `-1` is the right value to return here. This return value becomes the 
exit code of the `c-index-test` executable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to override preamble storage path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy updated this revision to Diff 500552.
vedgy retitled this revision from "[libclang] Add API to set preferred temp dir 
path" to "[libclang] Add API to override preamble storage path".
vedgy edited the summary of this revision.
vedgy added a comment.

Reimplement the API as discussed in review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/tools/c-index-test/c-index-test.c
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CIndexer.h
  clang/tools/libclang/libclang.map
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/libclang/LibclangTest.cpp
  clang/unittests/libclang/TestUtils.h

Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -22,11 +22,11 @@
 #include 
 
 class LibclangParseTest : public ::testing::Test {
-  // std::greater<> to remove files before their parent dirs in TearDown().
-  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> FilesAndDirsToRemove;
   std::string TestDir;
   bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
@@ -40,7 +40,7 @@
 TestDir = std::string(Dir.str());
 TUFlags = CXTranslationUnit_DetailedPreprocessingRecord |
   clang_defaultEditingTranslationUnitOptions();
-Index = clang_createIndex(0, 0);
+CreateIndex();
 ClangTU = nullptr;
   }
   void TearDown() override {
@@ -48,7 +48,7 @@
 clang_disposeIndex(Index);
 
 namespace fs = llvm::sys::fs;
-for (const std::string  : Files)
+for (const std::string  : FilesAndDirsToRemove)
   EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
 if (RemoveTestDirRecursivelyDuringTeardown)
   EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
@@ -63,7 +63,7 @@
FileI != FileEnd; ++FileI) {
 ASSERT_NE(*FileI, ".");
 path::append(Path, *FileI);
-Files.emplace(Path.str());
+FilesAndDirsToRemove.emplace(Path.str());
   }
   Filename = std::string(Path.str());
 }
@@ -101,6 +101,9 @@
 return string;
   };
 
+protected:
+  virtual void CreateIndex() { Index = clang_createIndex(0, 0); }
+
 private:
   template
   static CXChildVisitResult TraverseStateless(CXCursor cx, CXCursor parent,
Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "TestUtils.h"
 #include "clang-c/Index.h"
 #include "clang-c/Rewrite.h"
 #include "llvm/ADT/StringRef.h"
@@ -14,7 +15,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
-#include "TestUtils.h"
+#include 
 #include 
 #include 
 #include 
@@ -355,6 +356,115 @@
   clang_ModuleMapDescriptor_dispose(MMD);
 }
 
+class LibclangPreambleStorageTest : public LibclangParseTest {
+  std::string Main = "main.cpp";
+
+protected:
+  std::string PreambleDir;
+  void InitializePreambleDir() {
+llvm::SmallString<128> PathBuffer(TestDir);
+llvm::sys::path::append(PathBuffer, "preambles");
+namespace fs = llvm::sys::fs;
+ASSERT_FALSE(fs::create_directory(PathBuffer, false, fs::perms::owner_all));
+
+PreambleDir = static_cast(PathBuffer);
+FilesAndDirsToRemove.insert(PreambleDir);
+  }
+
+public:
+  void CountPreamblesInPreambleDir(int PreambleCount) {
+// For some reason, the preamble is not created without '\n' before `int`.
+WriteFile(Main, "\nint main() {}");
+
+TUFlags |= CXTranslationUnit_CreatePreambleOnFirstParse;
+ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0,
+ nullptr, 0, TUFlags);
+
+int FileCount = 0;
+
+namespace fs = llvm::sys::fs;
+std::error_code EC;
+for (fs::directory_iterator File(PreambleDir, EC), FileEnd;
+ File != FileEnd && !EC; File.increment(EC)) {
+  ++FileCount;
+
+  EXPECT_EQ(File->type(), fs::file_type::regular_file);
+
+  const auto Filename = llvm::sys::path::filename(File->path());
+  EXPECT_EQ(Filename.size(), std::strlen("preamble-%%.pch"));
+  EXPECT_TRUE(Filename.startswith("preamble-"));
+  EXPECT_TRUE(Filename.endswith(".pch"));
+
+  const auto Status = File->status();
+  

[PATCH] D143415: LibclangTest: remove libclang-test-* tmp dir reliably

[Igor Kushnir via Phabricator via cfe-commits]

vedgy updated this revision to Diff 500551.
vedgy added a comment.

Rebase on latest main branch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143415

Files:
  clang/unittests/libclang/LibclangTest.cpp
  clang/unittests/libclang/TestUtils.h


Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -14,18 +14,21 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
+#include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 #include 
-#include "gtest/gtest.h"
 
 class LibclangParseTest : public ::testing::Test {
-  std::set Files;
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
   std::string TestDir;
+  bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
   CXTranslationUnit ClangTU;
   unsigned TUFlags;
@@ -43,16 +46,26 @@
   void TearDown() override {
 clang_disposeTranslationUnit(ClangTU);
 clang_disposeIndex(Index);
+
+namespace fs = llvm::sys::fs;
 for (const std::string  : Files)
-  llvm::sys::fs::remove(Path);
-llvm::sys::fs::remove(TestDir);
+  EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
+if (RemoveTestDirRecursivelyDuringTeardown)
+  EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
+else
+  EXPECT_FALSE(fs::remove(TestDir, /*IgnoreNonExisting=*/false));
   }
   void WriteFile(std::string , const std::string ) {
 if (!llvm::sys::path::is_absolute(Filename)) {
   llvm::SmallString<256> Path(TestDir);
-  llvm::sys::path::append(Path, Filename);
+  namespace path = llvm::sys::path;
+  for (auto FileI = path::begin(Filename), FileEnd = path::end(Filename);
+   FileI != FileEnd; ++FileI) {
+ASSERT_NE(*FileI, ".");
+path::append(Path, *FileI);
+Files.emplace(Path.str());
+  }
   Filename = std::string(Path.str());
-  Files.insert(Filename);
 }
 llvm::sys::fs::create_directories(llvm::sys::path::parent_path(Filename));
 std::ofstream OS(Filename);
Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -515,6 +515,8 @@
   WriteFile(HeaderName, std::string(HeaderTop) + HeaderBottom);
   WriteFile(ModName, ModFile);
 
+  // Removing recursively is necessary to delete the module cache.
+  RemoveTestDirRecursivelyDuringTeardown = true;
   std::string ModulesCache = std::string("-fmodules-cache-path=") + TestDir;
   const char *Args[] = { "-fmodules", ModulesCache.c_str(),
  "-I", TestDir.c_str() };


Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -14,18 +14,21 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
+#include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 #include 
-#include "gtest/gtest.h"
 
 class LibclangParseTest : public ::testing::Test {
-  std::set Files;
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
   std::string TestDir;
+  bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
   CXTranslationUnit ClangTU;
   unsigned TUFlags;
@@ -43,16 +46,26 @@
   void TearDown() override {
 clang_disposeTranslationUnit(ClangTU);
 clang_disposeIndex(Index);
+
+namespace fs = llvm::sys::fs;
 for (const std::string  : Files)
-  llvm::sys::fs::remove(Path);
-llvm::sys::fs::remove(TestDir);
+  EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
+if (RemoveTestDirRecursivelyDuringTeardown)
+  EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
+else
+  EXPECT_FALSE(fs::remove(TestDir, /*IgnoreNonExisting=*/false));
   }
   void WriteFile(std::string , const std::string ) {
 if (!llvm::sys::path::is_absolute(Filename)) {
   llvm::SmallString<256> Path(TestDir);
-  llvm::sys::path::append(Path, Filename);
+  namespace path = llvm::sys::path;
+  for (auto FileI = path::begin(Filename), FileEnd = path::end(Filename);
+   FileI != FileEnd; ++FileI) {
+ASSERT_NE(*FileI, ".");
+path::append(Path, *FileI);
+Files.emplace(Path.str());
+  }
   Filename = std::string(Path.str());
-  Files.insert(Filename);
 }
 llvm::sys::fs::create_directories(llvm::sys::path::parent_path(Filename));
 

[PATCH] D143415: LibclangTest: remove libclang-test-* tmp dir reliably

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

@aaron.ballman, this test fix is independent from D143418 
 and can be reviewed separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143415

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4150360 , @aaron.ballman 
wrote:

> So my intuition is that the current behavior works well enough and I doubt 
> anyone's going to propose changes to it in the future.

So adding the `GlobalOptions` member to `CXIndexOptions`, adding the 
`clang_getDefaultGlobalOptions()` function and deprecating 
`clang_CXIndex_setGlobalOptions` in this revision is fine, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4147578 , @aaron.ballman 
wrote:

> In D143418#4131156 , @vedgy wrote:
>
>> On second thought, the proposed `clang_getDefaultGlobalOptions()` API 
>> already offers users a choice to either prefer or override each option value 
>> from the env. variables, just like the existing 
>> `clang_CXIndex_[gs]etGlobalOptions` API. The environment variables are 
>> binary options: an existence, not value, of an env. variable determines an 
>> initial option value. So I don't understand what are the two different ways 
>> to expose these options.
>
> I was thinking of the env variable as determining the initial option value, 
> so the two options I saw were "the the env variable provides the final 
> resulting value used by the program" and "the env variable provides the 
> default value used by the program". But you're right, the current behavior is 
> that the presence of the env variable determines the behavior, not the value 
> of the env variable.
>
> The question really boils down to: if the user passes in an option which says 
> "don't use thread background priority" to the call to create index, AND there 
> is an environment variable saying "use thread background priority", who 
> should win? And does the answer differ if the option says "use thread 
> background priority" and the environment variable does not exist?

The purpose of adding the `GlobalOptions` member to `CXIndexOptions` and 
deprecating `clang_CXIndex_setGlobalOptions` is to improve libclang's thread 
safety. The proposed approach keeps the meaning of the environment variables 
and enables straightforward porting of existing uses.

If someone decides to prioritize API users vs environment variables 
differently, then new 3-state (unset, disabled, enabled) environment variables 
could be introduced and (possibly) the existing ones deprecated. Such a change 
may require deprecating the proposed `clang_getDefaultGlobalOptions()` API 
addition and introducing a more informative replacement.

The question is whether such environment variable behavior changes are likely 
to be proposed any time soon and how important is this possibility compared to 
the potential thread safety improvement. I suppose if no one has proposed such 
environment variable changes yet, then the current behavior works well enough 
for all libclang users. Just searched for `LIBCLANG_BGPRIO_INDEX`, 
`LIBCLANG_BGPRIO_EDIT`, `CXGlobalOpt_ThreadBackgroundPriorityForIndexing`, 
`CXGlobalOpt_ThreadBackgroundPriorityForEditing`, 
`clang_CXIndex_setGlobalOptions` and `clang_CXIndex_getGlobalOptions` on the 
LLVM Project's issue tracker. Found no results for any of these strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4125756 , @aaron.ballman 
wrote:

>> 3. `clang_createIndex()` initializes global options based on environment 
>> variable values:
>>
>>   if (getenv("LIBCLANG_BGPRIO_INDEX"))
>>   CIdxr->setCXGlobalOptFlags(CIdxr->getCXGlobalOptFlags() |
>>  
>> CXGlobalOpt_ThreadBackgroundPriorityForIndexing);
>> if (getenv("LIBCLANG_BGPRIO_EDIT"))
>>   CIdxr->setCXGlobalOptFlags(CIdxr->getCXGlobalOptFlags() |
>>  
>> CXGlobalOpt_ThreadBackgroundPriorityForEditing);
>>
>> The recommended in documentation usage of `clang_CXIndex_setGlobalOptions` 
>> is:
>>
>>   * \code
>>   * CXIndex idx = ...;
>>   * clang_CXIndex_setGlobalOptions(idx,
>>   * clang_CXIndex_getGlobalOptions(idx) |
>>   * CXGlobalOpt_ThreadBackgroundPriorityForIndexing);
>>   * \endcode
>>
>> So making these options part of `struct CXIndexOptions` and deprecating 
>> `clang_CXIndex_setGlobalOptions` requires introducing another global 
>> function that would read the environment variables:
>>
>>   CINDEX_LINKAGE unsigned clang_getDefaultGlobalOptions();
>>
>> Is this the right approach?
>
> Hmm, to make this patch easier, I think we might want to leave the 
> environment variable behavior alone and not shift these into the options 
> structure (yet?). Naively, I think it makes sense for these to eventually 
> live in the options structure, but we could expose them in a few different 
> ways (an option to prefer the env variable over a manual value as it is today 
> or an option to prefer the manual value over the env variable for folks who 
> want more hermetic behavior). WDYT? My opinion here isn't super strong, so if 
> you have a strong desire to deprecate and add a replacement API, I think 
> that's a defensible course to take.

On second thought, the proposed `clang_getDefaultGlobalOptions()` API already 
offers users a choice to either prefer or override each option value from the 
env. variables, just like the existing `clang_CXIndex_[gs]etGlobalOptions` API. 
The environment variables are binary options: an existence, not value, of an 
env. variable determines an initial option value. So I don't understand what 
are the two different ways to expose these options.

In D143418#4130042 , @aaron.ballman 
wrote:

> In D143418#4126266 , @vedgy wrote:
>
>> 
>
> I've been trying to think of benefits for using a fixed-size integer type and 
> the closest I can come is the consistency of the structure size across 
> targets, but I don't think we need that consistency. I don't have a strong 
> preference for `unsigned` vs `size_t`, so how about we go with your slight 
> preference for `unsigned` unless someone finds a reason to use something else?

Sounds good. Here is my current WIP API version:

  typedef struct CXIndexOptions {
/**
 * The size of struct CIndexOptions used for option versioning.
 *
 * Always assign sizeof(CIndexOptions) to this member right after
 * creating a CXIndexOptions object.
 */
unsigned Size;
/**
 * \see clang_createIndex()
 */
int ExcludeDeclarationsFromPCH : 1;
int DisplayDiagnostics : 1;
/**
 * The path to a directory, in which to store temporary PCH files. If null 
or
 * empty, the default system temporary directory is used. These PCH files 
are
 * deleted on clean exit but stay on disk if the program crashes or is 
killed.
 *
 * Libclang does not create the directory at the specified path in the file
 * system. Therefore it must exist, or storing PCH files will fail.
 */
const char *PreambleStoragePath;
/**
 * Specifies a path which will contain log files for certain libclang
 * invocations. A null value implies that libclang invocations are not 
logged.
 */
const char *InvocationEmissionPath;
  } CXIndexOptions;
  
  /**
   * Provides a shared context for creating translation units.
   *
   * Call this function instead of clang_createIndex() if you need to configure
   * the additional options in CXIndexOptions.
   *
   * \returns The created index or null in case of error, such as an unsupported
   * value of options->Size.
   *
   * For example:
   * \code
   * CXIndex createIndex(const char *ApplicationTemporaryPath) {
   *   const int ExcludeDeclarationsFromPCH = 1;
   *   const int DisplayDiagnostics = 1;
   * #if CINDEX_VERSION_MINOR >= 64
   *   CIndexOptions Opts;
   *   Opts.Size = sizeof(CIndexOptions);
   *   Opts.ExcludeDeclarationsFromPCH = ExcludeDeclarationsFromPCH;
   *   Opts.DisplayDiagnostics = DisplayDiagnostics;
   *   Opts.PreambleStoragePath = ApplicationTemporaryPath;
   *   Opts.InvocationEmissionPath = NULL;
   *   CIndex Idx = clang_createIndexWithOptions();
   *   if (Idx)
   * return Idx;
   *   fprintf(stderr, "clang_createIndexWithOptions() 

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4125756 , @aaron.ballman 
wrote:

> In D143418#4125098 , @vedgy wrote:
>
>>> `uint32_t Size; // = sizeof(struct CIndexOptions), used for option 
>>> versioning`
>>
>> 1. `uint32_t` was introduced in C99. Can/should it be used in //Index.h//? 
>> Only built-in `[unsigned] (int|long)` types are currently used in this file.
>
> That is a really good question. I couldn't spot anything existing in the 
> header file that requires C99 or later (no // comments, no C99 types like 
> _Bool or uint32_t, etc). I think the common pattern would be to use 
> `unsigned`, which is also variably-sized (as are all the integer types, 
> technically, thanks to CHAR_BIT), but we do have one existing use of `size_t` 
> in the file, which is probably the best type to use there given that we 
> expect people to assign the results of `sizeof` into it. WDYT about using 
> `size_t`?
>
> I don't have the historical context to know whether we expect this header to 
> be C89 compatible, so it's not clear to me how disruptive it would be to use 
> stdint.h. One alternative would be to use stdint.h if it's available (via 
> `__has_include`) and otherwise fallback on a C89 custom definition of 
> uint32_t, but that strikes me as being more likely to introduce problems on 
> systems we don't test on or for compilers we don't test with.

`size_t` indeed makes logical sense for this member as that's the return type 
of `sizeof`. `size_t` is two times larger than `unsigned` on x86_64, but I 
don't think the size of this struct has any chance of impacting performance. 
Though it wouldn't hurt to pack the size and the boolean options in a 
single-pointer-sized region on x86_64. After all, this struct's size will never 
reach `UINT_MAX`. I slightly prefer `unsigned` due to my efficiency 
inclinations :). What do you prefer? Is there any benefit in using a fixed-size 
integer type here?

>> 2. Should `int excludeDeclarationsFromPCH` and `int displayDiagnostics` 
>> currently passed to `clang_createIndex()` also be included in the struct? 
>> Then only a single argument will be passed to 
>> `clang_createIndexWithOptions()`: `CXIndexOptions`.
>
> I think that makes sense to me. It does raise the question of whether we want 
> to pack these boolean-like fields together, as in:
>
>   struct CXIndexOptions {
> size_t Size;
>   
> int ExcludeDeclsFromPCH : 1;
> int DisplayDiagnostics : 1;
> int Reserved : 30;
>   
> const char *PreambleStoragePath;
> ...
>   };
>
> This makes it a little less likely to need to grow the structure when adding 
> new options.

When we add new options, the struct's size must grow in order to distinguish 
different struct versions and prevent undefined behavior! If a member is added 
within the same LLVM release, it and other members added in that release can be 
reordered and packed to minimize the size. For example, I plan to add a `bool 
StorePreamblesInMemory` in LLVM 17. While adding it, I can reorder and pack 
anything I like as the whole struct is introduced in this version. But there is 
no need to reserve anything, I think. This is assuming we don't need to support 
compatibility at each intermediate commit of libclang.

>> 3. `clang_createIndex()` initializes global options based on environment 
>> variable values:
>>
>>   if (getenv("LIBCLANG_BGPRIO_INDEX"))
>>   CIdxr->setCXGlobalOptFlags(CIdxr->getCXGlobalOptFlags() |
>>  
>> CXGlobalOpt_ThreadBackgroundPriorityForIndexing);
>> if (getenv("LIBCLANG_BGPRIO_EDIT"))
>>   CIdxr->setCXGlobalOptFlags(CIdxr->getCXGlobalOptFlags() |
>>  
>> CXGlobalOpt_ThreadBackgroundPriorityForEditing);
>>
>> The recommended in documentation usage of `clang_CXIndex_setGlobalOptions` 
>> is:
>>
>>   * \code
>>   * CXIndex idx = ...;
>>   * clang_CXIndex_setGlobalOptions(idx,
>>   * clang_CXIndex_getGlobalOptions(idx) |
>>   * CXGlobalOpt_ThreadBackgroundPriorityForIndexing);
>>   * \endcode
>>
>> So making these options part of `struct CXIndexOptions` and deprecating 
>> `clang_CXIndex_setGlobalOptions` requires introducing another global 
>> function that would read the environment variables:
>>
>>   CINDEX_LINKAGE unsigned clang_getDefaultGlobalOptions();
>>
>> Is this the right approach?
>
> Hmm, to make this patch easier, I think we might want to leave the 
> environment variable behavior alone and not shift these into the options 
> structure (yet?). Naively, I think it makes sense for these to eventually 
> live in the options structure, but we could expose them in a few different 
> ways (an option to prefer the env variable over a manual value as it is today 
> or an option to prefer the manual value over the env variable for folks who 
> want more hermetic behavior). WDYT? My opinion here isn't super strong, so if 
> you have a strong desire to 

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

> `uint32_t Size; // = sizeof(struct CIndexOptions), used for option versioning`

`uint32_t` was introduced in C99. Can/should it be used in //Index.h//? Only 
built-in `[unsigned] (int|long)` types are currently used in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4122821 , @aaron.ballman 
wrote:

>> How about including existing options, which //should// be set in 
>> constructor, in the initial struct version and deprecating the corresponding 
>> setters?
>
> I think that makes a lot of sense.

How to deprecate the setters? Add `DEPRECATED` in the documentations as is 
already done in two places in //Index.h//?

>>   `const char *preferredTempDirPath;`
>
> In terms of documenting the structure, should we document this member as only 
> impacting preamble files currently, should we rename this to be preamble-path 
> specific, or should we try to use this preferred path in all the places we 
> need the temp directory?
>
> (I lean towards renaming to preamble-path specific -- then users don't have 
> the problem of upgrading to a newer CIndex potentially changing the behavior 
> of where non-preamble files are stored, and we don't have to do the work 
> up-front to audit other temp file uses.)

Looks like an imperfect general implementation is unacceptable. So I'll rename 
this data member to `PreambleStoragePath`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D143418#4118905 , @aaron.ballman 
wrote:

> I don't think "tries hard" is a good enough guarantee for where files are 
> placed. I'm not comfortable with the security posture of that approach as it 
> could potentially leak sensitive information (try to override the temp 
> directory to something that's chroot jailed, get sensitive files showing up 
> in the system temp directory anyway).

That's why the function's documentation explicitly doesn't guarantee anything. 
It should be safe to assume that security-sensitive users would at least read 
the documentation. If this function and potential future function like it are 
named specifically, a responsible security use case wouldn't be better off. The 
only safety advantage would be to those who don't bother reading the 
documentation. But why should we care much about such hypothetical careless 
security needs?

>> Does the multithreading issue mean that `clang_parseTranslationUnit_Impl()` 
>> can be called in a non-main thread and thus concurrently with 
>> `clang_CXIndex_setPreferredTempDirPath()`?
>
> Yes. However, I don't think that situation is officially supported (it's more 
> that we don't want to knowingly make it harder to support in the future).

All right. Let's do it via a new constructor then. Unfortunately, supporting 
different `CXIndexOptions` struct sizes/versions would complicate the libclang 
implementation and the libclang user code. But the alternative of adding a new 
constructor function each time can either grow to a large number of function 
parameters unneeded by most users or to an exponential number of overloads that 
support different usage requirements.

How about including existing options, which //should// be set in constructor, 
in the initial struct version and deprecating the corresponding setters?

  typedef struct CXIndexOptions {
uint32_t size; // sizeof(struct CIndexOptions), used for option versioning
unsigned globalOptions;
const char *preferredTempDirPath;
const char *invocationEmissionPath;
  } CXIndexOptions;

The naming of struct data members is inconsistent in libclang's Index.h. They 
start with a lower-case letter in about half of the structs. Which naming 
scheme should the members of the new struct use?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143755: [clang-format] Add a space between an overloaded operator and '>'

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Hi @owenpan. Thank you for fixing this bug!
Have you noticed this paragraph in my bug report?

> I believe `clang_getTypeSpelling()`, or more likely `QualType::print()` used 
> by it, should insert a tab character between such tokens to pretty-print 
> compilable code. The tab character is preferable to the space character here, 
> because the users may rely on the fact that pretty-printed binary operators 
> are surrounded by spaces to distinguish them from angle brackets.

KDevelop parses the result of `clang_getTypeSpelling()` when libclang API is 
lacking. Since this recent commit 

 KDevelop's parsing relies on the empirical fact that only operators are 
surrounded by spaces to distinguish them from angle brackets. Does this 
revision introduce angle brackets surrounded by spaces? Can tab characters be 
used instead? If not, do you know how else such angle brackets can be 
distinguished from operators?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143755

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a subscriber: dblaikie.
vedgy added a comment.

In D143418#4116290 , @aaron.ballman 
wrote:

>> So how about a more sincere general solution: setPreferredTempDirPath()? The 
>> documentation would say that libclang tries its best to place temporary 
>> files into the specified directory, but some might end up in the system 
>> temporary directory instead.
>
> I don't think those semantics are reasonable. "Tries its best" is fine for 
> things that are heuristic based, like deciding when to do an analysis cut 
> point, but is too non-deterministic for something like "where do files get 
> placed". I think it's reasonable that we either:
>
> 1. expose an option to say "I prefer preambles to be put in this directory" 
> for libclang, or
> 2. expose an option to say "I prefer using this directory over the temp 
> directory for all files" for libclang,
>
> I thought we agreed that #2 is too high of risk because it requires auditing 
> libclang for all the places it decides to put something into the temp 
> directory, and so we were going with #1 and doing it as part of the cindex 
> constructor so that users cannot run into the surprise issues 
> (multithreading, files changing location mid-run) with an independent setter.

This revision implements #2, but warns about possible imperfections of the 
implementation. KDevelop (and other potential users of the added API that need 
to override the temp dir location for the same reason) is not interested in 
overriding each temp dir usage separately. For this only known use case an 
imperfectly implemented `clang_CXIndex_setPreferredTempDirPath()` is much more 
convenient than a perfectly correct `clang_CXIndex_setPreambleStoragePath()`. 
With the general API name, if another temp dir use is overridden in the future, 
neither libclang API nor the code that uses it would have to be adjusted.

In D139774#4066519 , @dblaikie wrote:

> In D139774#4065753 , @aaron.ballman 
> wrote:
>
>> From a design perspective, my preference is to not add new APIs at the file 
>> system support layer in LLVM to override the temporary directory but instead 
>> allow individual components to override the decision where to put files. 
>> Overriding a system directory at the LLVM support level feels unclean to me 
>> because 1) threading concerns (mostly related to lock performance), 2) 
>> consistency concerns (put files in temp directory, override temp directory, 
>> put additional files in the new directory), 3) principle of least surprise. 
>> To me, the LLVM layer is responsible for interfacing with the system and 
>> asking for the system temp directory should get you the same answer as 
>> querying for that from the OS directly.
>
> I've mixed feelings about this, but yeah, I guess the issues with global 
> state are pretty undesirable. I don't worry too much about (2) - doesn't feel 
> too problematic to me (for any individual file, presumably the implementation 
> recorded the name of the file if they were going to access it again - so 
> still be using the old path if necessary).
>
> But, yeah, lack of any "system" abstraction that could be passed into all the 
> various LLVM/MC/AST contexts to swap out the implementation limits the 
> options a bit to more ad-hoc/case-by-case solutions. I feel like that's not a 
> great solution to KDevelop's general problem, though - they're dealing with 
> one particularly big file, but it doesn't feel very principled to me to fix 
> that one compared to all the other (albeit smaller) temp files & maybe at 
> some point in the future some bigger temp files are created elsewhere...

Based on this comment and on current preamble storage path code, I thought 
files-changing-location-mid-run should not be a problem.

Does the multithreading issue mean that `clang_parseTranslationUnit_Impl()` can 
be called in a non-main thread and thus concurrently with 
`clang_CXIndex_setPreferredTempDirPath()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140756: Add clang_CXXMethod_isExplicit to libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D140756#4115381 , @aaron.ballman 
wrote:

> The spare moment came sooner than expected, I've made the changes in 
> de4321cf2cf28f2bae7fc0f1897957e73b726f89 
> .

Isn't the `global:` label needed in the new `LLVM_17` block/tag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

I hoped to avoid applying an unrelated `git clang-format`'s include reordering, 
but the CI fails because of that. Will apply it along with changes requested 
during code review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy updated this revision to Diff 495372.
vedgy edited the summary of this revision.
vedgy added a comment.

Address an old inline review comment
https://reviews.llvm.org/D139774#inline-1360634 and add a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143418

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CIndexer.h
  clang/tools/libclang/libclang.map
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/libclang/LibclangTest.cpp
  clang/unittests/libclang/TestUtils.h

Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -22,11 +22,11 @@
 #include 
 
 class LibclangParseTest : public ::testing::Test {
-  // std::greater<> to remove files before their parent dirs in TearDown().
-  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> FilesAndDirsToRemove;
   std::string TestDir;
   bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
@@ -48,7 +48,7 @@
 clang_disposeIndex(Index);
 
 namespace fs = llvm::sys::fs;
-for (const std::string  : Files)
+for (const std::string  : FilesAndDirsToRemove)
   EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
 if (RemoveTestDirRecursivelyDuringTeardown)
   EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
@@ -63,7 +63,7 @@
FileI != FileEnd; ++FileI) {
 ASSERT_NE(*FileI, ".");
 path::append(Path, *FileI);
-Files.emplace(Path.str());
+FilesAndDirsToRemove.emplace(Path.str());
   }
   Filename = std::string(Path.str());
 }
Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -15,6 +15,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 #include "TestUtils.h"
+#include 
 #include 
 #include 
 #include 
@@ -355,6 +356,94 @@
   clang_ModuleMapDescriptor_dispose(MMD);
 }
 
+class LibclangTempDirTest : public LibclangParseTest {
+public:
+  std::string Main = "main.cpp";
+  std::string TempDir;
+
+  void SetUp() override {
+LibclangParseTest::SetUp();
+TUFlags |= CXTranslationUnit_CreatePreambleOnFirstParse;
+// For some reason, the preamble is not created without '\n' before `int`.
+WriteFile(Main, "\nint main() {}");
+
+llvm::SmallString<128> TempDirBuffer(TestDir);
+llvm::sys::path::append(TempDirBuffer, "temp_dir");
+namespace fs = llvm::sys::fs;
+ASSERT_FALSE(
+fs::create_directory(TempDirBuffer, false, fs::perms::owner_all));
+
+TempDir = static_cast(TempDirBuffer);
+FilesAndDirsToRemove.insert(TempDir);
+  }
+  void CheckPreamblesInTempDir(int PreambleCount) {
+ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0,
+ nullptr, 0, TUFlags);
+
+int FileCount = 0;
+
+namespace fs = llvm::sys::fs;
+std::error_code EC;
+for (fs::directory_iterator File(TempDir, EC), FileEnd;
+ File != FileEnd && !EC; File.increment(EC)) {
+  ++FileCount;
+
+  EXPECT_EQ(File->type(), fs::file_type::regular_file);
+
+  const auto Filename = llvm::sys::path::filename(File->path());
+  EXPECT_EQ(Filename.size(), std::strlen("preamble-%%.pch"));
+  EXPECT_TRUE(Filename.startswith("preamble-"));
+  EXPECT_TRUE(Filename.endswith(".pch"));
+
+  const auto Status = File->status();
+  ASSERT_TRUE(Status);
+  if (false) {
+// The permissions assertion below fails, because the .pch.tmp file is
+// created with default permissions and replaces the .pch file along
+// with its permissions. Therefore the permissions set in
+// TempPCHFile::create() don't matter in the end.
+EXPECT_EQ(Status->permissions(), fs::owner_read | fs::owner_write);
+  }
+}
+
+EXPECT_EQ(FileCount, PreambleCount);
+  }
+};
+
+TEST_F(LibclangTempDirTest, clang_CXIndex_setPreferredTempDirPath_notCalled) {
+  CheckPreamblesInTempDir(0);
+}
+
+TEST_F(LibclangTempDirTest, clang_CXIndex_setPreferredTempDirPath_toNull) {
+  clang_CXIndex_setPreferredTempDirPath(Index, nullptr);
+  CheckPreamblesInTempDir(0);
+}
+
+TEST_F(LibclangTempDirTest,
+   clang_CXIndex_setPreferredTempDirPath_toEmptyString) {
+  clang_CXIndex_setPreferredTempDirPath(Index, 

[PATCH] D140756: Add clang_CXXMethod_isExplicit to libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

The added, then reverted release note was lost when this revision landed the 
second time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy abandoned this revision.
vedgy added a comment.

D143418  implements my latest idea described 
in the previous comment. Let us continue the discussion there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143418: [libclang] Add API to set preferred temp dir path

[Igor Kushnir via Phabricator via cfe-commits]

vedgy created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
vedgy requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added projects: clang, clang-tools-extra.

TempPCHFile::create() calls llvm::sys::fs::createTemporaryFile() to
create a file named preamble-*.pch in a system temporary directory. This
commit allows to override the directory where these often many and large
preamble-*.pch files are stored. The added API is named generally to
store other temporary files created by libclang in the same overridden
preferred temporary directory, once such other files are discovered.

A libclang user passes a preferred temporary directory path to
clang_CXIndex_setPreferredTempDirPath(), which stores it in
CIndexer::PreferredTempDirPath. Whenever
clang_parseTranslationUnit_Impl() is called, it passes
CIndexer::PreferredTempDirPath to ASTUnit::LoadFromCommandLine(), which
stores this argument in ASTUnit::PreferredTempDirPath. Whenever
ASTUnit::getMainBufferWithPrecompiledPreamble() is called, it passes
ASTUnit::PreferredTempDirPath to PrecompiledPreamble::Build().
PrecompiledPreamble::Build() forwards the corresponding StoragePath
argument to TempPCHFile::create(). If StoragePath is not empty,
TempPCHFile::create() stores the preamble-*.pch file in the directory at
the specified path rather than in the system temporary directory.

The analysis below proves that this passing around of the
PreferredTempDirPath string is sufficient to guarantee that the libclang
user override is used in TempPCHFile::create(). The analysis ignores API
uses in test code.

TempPCHFile::create() is called only in PrecompiledPreamble::Build().
PrecompiledPreamble::Build() is called only in two places: one in
clangd, which is not used by libclang, and one in
ASTUnit::getMainBufferWithPrecompiledPreamble().
ASTUnit::getMainBufferWithPrecompiledPreamble() is called in 3 places:

1. ASTUnit::LoadFromCompilerInvocation() [analyzed below].
2. ASTUnit::Reparse(), which in turn is called only from

clang_reparseTranslationUnit_Impl(), which in turn is called only from
clang_reparseTranslationUnit(). clang_reparseTranslationUnit() is never
called in LLVM code, but is part of public libclang API. This function's
documentation requires its translation unit argument to have been built
with clang_createTranslationUnitFromSourceFile().
clang_createTranslationUnitFromSourceFile() delegates its work to
clang_parseTranslationUnit(), which delegates to
clang_parseTranslationUnit2(), which delegates to
clang_parseTranslationUnit2FullArgv(), which delegates to
clang_parseTranslationUnit_Impl(), which passes
CIndexer::PreferredTempDirPath to the ASTUnit it creates.

3. ASTUnit::CodeComplete() passes AllowRebuild = false to

ASTUnit::getMainBufferWithPrecompiledPreamble(), which makes it return
nullptr before calling PrecompiledPreamble::Build().

Both ASTUnit::LoadFromCompilerInvocation() overloads (one of which
delegates its work to another) call
ASTUnit::getMainBufferWithPrecompiledPreamble() only if their argument
PrecompilePreambleAfterNParses > 0. LoadFromCompilerInvocation() is
called in:

1. ASTBuilderAction::runInvocation() keeps the default parameter value

of PrecompilePreambleAfterNParses = 0, meaning that the preamble file is
never created from here.

2. ASTUnit::LoadFromCommandLine().

ASTUnit::LoadFromCommandLine() is called in two places:

1. CrossTranslationUnitContext::ASTLoader::loadFromSource() keeps the

default parameter value of PrecompilePreambleAfterNParses = 0, meaning
that the preamble file is never created from here.

2. clang_parseTranslationUnit_Impl(), which passes

CIndexer::PreferredTempDirPath to the ASTUnit it creates.

TempPCHFile::create() uses PreferredTempDirPath in the same way as
LibclangInvocationReporter() uses InvocationEmissionPath. The
documentation for clang_CXIndex_setInvocationEmissionPathOption() does
not specify ownership, encoding, relative vs absolute path or separator
requirements. So the new function's documentation doesn't either.

The added function clang_CXIndex_setPreferredTempDirPath() works as
expected in KDevelop:
https://invent.kde.org/kdevelop/kdevelop/-/merge_requests/283

Fixes: https://github.com/llvm/llvm-project/issues/51847

Depends on D143415 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143418

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CIndexer.h
  clang/tools/libclang/libclang.map
  clang/unittests/Frontend/ASTUnitTest.cpp
  clang/unittests/libclang/LibclangTest.cpp
  clang/unittests/libclang/TestUtils.h

Index: clang/unittests/libclang/TestUtils.h

[PATCH] D143415: LibclangTest: remove libclang-test-* tmp dir reliably

[Igor Kushnir via Phabricator via cfe-commits]

vedgy created this revision.
Herald added a project: All.
vedgy requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Temporary directories created by two LibclangReparseTest tests -
ReparseWithModule and clang_parseTranslationUnit2FullArgv - remained in
the system temporary directory after running libclangTests, because not
all files and subdirectories created in TestDir were added to set
LibclangParseTest::Files.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143415

Files:
  clang/unittests/libclang/LibclangTest.cpp
  clang/unittests/libclang/TestUtils.h


Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -14,18 +14,21 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
+#include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 #include 
-#include "gtest/gtest.h"
 
 class LibclangParseTest : public ::testing::Test {
-  std::set Files;
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
   std::string TestDir;
+  bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
   CXTranslationUnit ClangTU;
   unsigned TUFlags;
@@ -43,16 +46,26 @@
   void TearDown() override {
 clang_disposeTranslationUnit(ClangTU);
 clang_disposeIndex(Index);
+
+namespace fs = llvm::sys::fs;
 for (const std::string  : Files)
-  llvm::sys::fs::remove(Path);
-llvm::sys::fs::remove(TestDir);
+  EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
+if (RemoveTestDirRecursivelyDuringTeardown)
+  EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
+else
+  EXPECT_FALSE(fs::remove(TestDir, /*IgnoreNonExisting=*/false));
   }
   void WriteFile(std::string , const std::string ) {
 if (!llvm::sys::path::is_absolute(Filename)) {
   llvm::SmallString<256> Path(TestDir);
-  llvm::sys::path::append(Path, Filename);
+  namespace path = llvm::sys::path;
+  for (auto FileI = path::begin(Filename), FileEnd = path::end(Filename);
+   FileI != FileEnd; ++FileI) {
+ASSERT_NE(*FileI, ".");
+path::append(Path, *FileI);
+Files.emplace(Path.str());
+  }
   Filename = std::string(Path.str());
-  Files.insert(Filename);
 }
 llvm::sys::fs::create_directories(llvm::sys::path::parent_path(Filename));
 std::ofstream OS(Filename);
Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -515,6 +515,8 @@
   WriteFile(HeaderName, std::string(HeaderTop) + HeaderBottom);
   WriteFile(ModName, ModFile);
 
+  // Removing recursively is necessary to delete the module cache.
+  RemoveTestDirRecursivelyDuringTeardown = true;
   std::string ModulesCache = std::string("-fmodules-cache-path=") + TestDir;
   const char *Args[] = { "-fmodules", ModulesCache.c_str(),
  "-I", TestDir.c_str() };


Index: clang/unittests/libclang/TestUtils.h
===
--- clang/unittests/libclang/TestUtils.h
+++ clang/unittests/libclang/TestUtils.h
@@ -14,18 +14,21 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
+#include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 #include 
-#include "gtest/gtest.h"
 
 class LibclangParseTest : public ::testing::Test {
-  std::set Files;
+  // std::greater<> to remove files before their parent dirs in TearDown().
+  std::set> Files;
   typedef std::unique_ptr fixed_addr_string;
   std::map UnsavedFileContents;
 public:
   std::string TestDir;
+  bool RemoveTestDirRecursivelyDuringTeardown = false;
   CXIndex Index;
   CXTranslationUnit ClangTU;
   unsigned TUFlags;
@@ -43,16 +46,26 @@
   void TearDown() override {
 clang_disposeTranslationUnit(ClangTU);
 clang_disposeIndex(Index);
+
+namespace fs = llvm::sys::fs;
 for (const std::string  : Files)
-  llvm::sys::fs::remove(Path);
-llvm::sys::fs::remove(TestDir);
+  EXPECT_FALSE(fs::remove(Path, /*IgnoreNonExisting=*/false));
+if (RemoveTestDirRecursivelyDuringTeardown)
+  EXPECT_FALSE(fs::remove_directories(TestDir, /*IgnoreErrors=*/false));
+else
+  EXPECT_FALSE(fs::remove(TestDir, /*IgnoreNonExisting=*/false));
   }
   void WriteFile(std::string , const std::string ) {
 if (!llvm::sys::path::is_absolute(Filename)) {
   llvm::SmallString<256> Path(TestDir);
-  llvm::sys::path::append(Path, Filename);
+  namespace path = llvm::sys::path;
+  for (auto FileI = path::begin(Filename), FileEnd = path::end(Filename);
+   FileI != 

[PATCH] D140756: Add clang_CXXMethod_isExplicit to libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added inline comments.



Comment at: clang/tools/libclang/libclang.map:419
 clang_getSymbolGraphForUSR;
+clang_CXXMethod_isExplicit;
 };

This line should be moved from under the `LLVM_16` tag to under a new `LLVM_17` 
tag. Alternatively this commit can be cherry-picked into the LLVM 16 branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140756: Add clang_CXXMethod_isExplicit to libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

I meant that the commit message of 
https://reviews.llvm.org/rG79571aa2103c95760a07e3549d8636379e4948f0 
misleadingly refers to this review's commit. But `CINDEX_VERSION_MINOR == 63` 
is for previous commits. This commit will require incrementing 
`CINDEX_VERSION_MINOR` again to `64` in Clang 17. Hopefully my preamble-storage 
patches will also be included in Clang 17 and share the same minor version `64` 
:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140756: Add clang_CXXMethod_isExplicit to libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

> 79571aa2103c95760a07e3549d8636379e4948f0 
>  is the 
> commit on main and https://github.com/llvm/llvm-project/issues/60478 is for 
> the 16.x cherry-pick.

Thank you Aaron! But note that this review's commit ended up in Clang 17, not 
Clang 16. Previous API additions have been implemented earlier and are included 
in Clang 16.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140756: Add clang_CXXMethod_isExplicit to libclang

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Hello Luca and Aaron.
I have noticed that you recently implemented/reviewed multiple interesting 
libclang features, some of which can be used in KDevelop. However, 
`CINDEX_VERSION_MINOR` was last modified in Clang 13. This constant's 
documentation states:

> `CINDEX_VERSION_MINOR` should increase when there are API additions.

Can this constant be incremented in the upcoming Clang 16 release?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Another disadvantage of using `CINDEX_VERSION_MINOR` instead of the `sizeof` is 
that an older libclang version won't know about compatibility of newer versions 
with itself. But this reasoning brought me to a different thought: when a 
program is compiled against a libclang version X, it should not be expected to 
be runtime-compatible with a libclang version older than X. For example, 
KDevelop uses `CINDEX_VERSION_MINOR` to decide whether or not certain libclang 
API is available.

I suspect modifying the exposed struct's size will violate ODR in C++ programs 
that use libclang. Quote from the cppreference ODR page 
:

> Note: in C, there is no program-wide ODR for types, and even extern 
> declarations of the same variable in different translation units may have 
> different types as long as they are compatible. In C++, the source-code 
> tokens used in declarations of the same type must be the same as described 
> above: if one .cpp file defines `struct S { int x; };` and the other .cpp 
> file defines `struct S { int y; };`, the behavior of the program that links 
> them together is undefined. This is usually resolved with unnamed namespaces.

I think the answer to this StackOverflow question 

 confirms my suspicion.

In order to avoid the undefined behavior, libclang can use the same approach as 
for other structs/classes: make the struct opaque and provide functions 
`clang_createIndexOptions()`,  `clang_disposeIndexOptions()`, 
`clang_setTempDirPath(CXIndexOptions options, const char *path)`.

`int excludeDeclarationsFromPCH` and `int displayDiagnostics` currently passed 
to `clang_createIndex()` can also be included in the struct and acquire their 
own setter functions. Then only a single argument will be passed to 
`clang_createIndexWithOptions()`: `CXIndexOptions`.

--

In D139774#4094619 , @aaron.ballman 
wrote:

> In D139774#4092553 , @vedgy wrote:
>
>> In D139774#4091631 , 
>> @aaron.ballman wrote:
>>
>>> My preference is still for specific API names. If we want to do something 
>>> general to all temp files, I think `FileSystemOptions` is the way to go.
>>>
>>> My concern with not using a constructor is that, because this is the C API, 
>>> we will be supporting the functionality for a long time even if we do 
>>> switch to using a global override in `FileSystemOptions` where you would 
>>> need to set the information up before executing the compiler. To me, these 
>>> paths are something that should not be possible to change once the compiler 
>>> has started executing. (That also solves the multithreading problem where 
>>> multiple threads are fighting over what the temp directory should be and 
>>> stepping on each other's toes.)
>>
>> As I understand, this leaves two choices:
>>
>> 1. Specific preamble API names, two separate non-constructor setters; the 
>> option values are stored in a separate struct (or even as two separate data 
>> members), not inside `FileSystemOptions`.
>> 2. General temporary-storage arguments (possibly combined in a struct) to a 
>> new overloaded constructor function; the option values are stored inside the 
>> `FileSystemOptions` struct.
>>
>> The second alternative is likely more difficult to implement, more risky and 
>> less convenient to use (the store-in-memory bool option cannot be modified 
>> at any time). Perhaps it should be delayed until (and if) we learn of other 
>> temporary files libclang creates? A downside of implementing the first 
>> option now is that the specific API would have to be supported for a long 
>> time, even after the general temporary-file API is implemented.
>
> I still think the general solution is where we ultimately want to get to and 
> that makes me hesitant to go with specific preamble API names despite that 
> being the direction you prefer. If this wasn't the C APIs, I'd be less 
> concerned because we make no stability guarantees about our C++ interfaces. 
> But we try really hard not to break the C APIs, so adding the specific names 
> is basically committing to not only the APIs but their semantics. I think 
> that makes implementing the general solution slightly more awkward because 
> these are weird special cases that barely get tested in-tree, so they'd be 
> very easy to overlook and accidentally break.
>
> Is there a middle ground where, instead of #2 for general temporary storage, 
> we went with #2 but with compiler-specific directories instead of system 
> directories. e.g., don't let the caller set the temp directory, but do let 
> the caller set the preamble directory (which defaults to the temp directory) 
> so long as it's set before invoking the 

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Perhaps the struct should contain the value of `CINDEX_VERSION_MINOR` instead 
of the `sizeof`? This way, libclang can change the meaning of a struct member 
without changing the size of the struct. For example, replace 
`PreambleStoragePath` with `TemporaryDirectoryPath`. Such a change could even 
be quiet, because setting the more general temporary directory path would 
likely be welcome or at least acceptable to the API users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4096695 , @aaron.ballman 
wrote:

> There's three scenarios when a field is added to the structure: 1) library 
> and app are matching versions, 2) library is newer than app, 3) app is newer 
> than library. #1 is the happy path most folks will hopefully be on. For #2 
> and #3, the app will either be sending more or less data than the library 
> expects, but the library will know how much of the structure is valid based 
> on the size field. If the size is too small and the library can't get data it 
> needs, it can catch that and report an error instead of reading past a 
> buffer. And if the size is too large, the library can ignore anything it 
> doesn't know about or it can give an error due to not knowing how to support 
> the semantics of the newer interface.

In the second case, the library ideally should assume that the missing struct 
members are not specified and behave as the corresponding older version. In the 
third case, the app can support older libclang versions by passing successively 
smaller structs until libclang returns a valid `CIndex`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4096527 , @aaron.ballman 
wrote:

> That sounds like a good plan to me. I wonder if we want to name it something 
> like `clang_createIndexWithOptions` (or something generic like that), and 
> give it a versioned structure of options along the lines of:
>
>   struct CIndexOptions {
> uint32_t Size; // sizeof(struct CIndexOptions), used for option versioning
> const char *PreambleStoragePath; // This pointer can be freed after 
> creating the index
>   };
>
> and define the function to return an error if `Size < sizeof(struct 
> CIndexOptions)`. This should allow us to add additional options to the 
> structure without having to introduce a new constructor API each time.

Would this be the recommended usage of such an API?

1. Call `clang_createIndexWithOptions`.
2. If it returns `nullptr` index, report an error in the application (e.g. 
print a warning or show an error in the UI) and fall back to code paths that 
support older Clang versions, beginning with calling the older constructor 
`clang_createIndex`.

Is assigning `sizeof(CIndexOptions)` to `Size` the API user's responsibility or 
should libclang define an inline function `clang_initializeCIndexOptions`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4094619 , @aaron.ballman 
wrote:

> Is there a middle ground where, instead of #2 for general temporary storage, 
> we went with #2 but with compiler-specific directories instead of system 
> directories. e.g., don't let the caller set the temp directory, but do let 
> the caller set the preamble directory (which defaults to the temp directory) 
> so long as it's set before invoking the compiler? This still won't let you 
> change options mid-run, but it also seems like it should have less risk of 
> affecting other components while still solving the thread safety issues. I'm 
> not certain if it's any easier to implement, but I think it does save you 
> from modifying `FileSystemOptions`. As a separate item, we could then 
> consider adding a new C API to let you toggle the in-memory vs on-disk 
> functionality after exploring that it won't cause other problems because 
> nobody considered the possibility that it's not a stable value for the 
> compiler invocation.

OK, so I'm going to implement overriding the preamble directory in 
`clang_createIndexWithPreambleStoragePath` (or `clang_createIndex2` or 
`clang_createIndexExt` or?); try to keep it simple and not modify 
`FileSystemOptions`; deal with the in-memory option separately later. Abandon 
this revision now and create another one once ready?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4091631 , @aaron.ballman 
wrote:

> My preference is still for specific API names. If we want to do something 
> general to all temp files, I think `FileSystemOptions` is the way to go.
>
> My concern with not using a constructor is that, because this is the C API, 
> we will be supporting the functionality for a long time even if we do switch 
> to using a global override in `FileSystemOptions` where you would need to set 
> the information up before executing the compiler. To me, these paths are 
> something that should not be possible to change once the compiler has started 
> executing. (That also solves the multithreading problem where multiple 
> threads are fighting over what the temp directory should be and stepping on 
> each other's toes.)

As I understand, this leaves two choices:

1. Specific preamble API names, two separate non-constructor setters; the 
option values are stored in a separate struct (or even as two separate data 
members), not inside `FileSystemOptions`.
2. General temporary-storage arguments (possibly combined in a struct) to a new 
overloaded constructor function; the option values are stored inside the 
`FileSystemOptions` struct.

The second alternative is likely more difficult to implement, more risky and 
less convenient to use (the store-in-memory bool option cannot be modified at 
any time). Perhaps it should be delayed until (and if) we learn of other 
temporary files libclang creates? A downside of implementing the first option 
now is that the specific API would have to be supported for a long time, even 
after the general temporary-file API is implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

3 out of 4 alternatives remain:

> 1. Add an option to store the preamble-*.pch files in RAM instead of /tmp and 
> add a corresponding option in KDevelop configuration UI. This would work 
> perfectly for me, provided I don't change my mind and decide to turn this 
> option off, in which case I'll be back to square one.
> 2. Add an option to store the preamble-*.pch files in a custom directory 
> instead of a general temporary directory. The option could be named generally 
> (2a: overrideTemporaryDirectory) or specially (2b: setPreambleStoragePath). 
> If the option is named generally, other temporary files created by libclang 
> could be identified in the future and placed in the same directory without 
> changing the API.
> 3. 1 and 2 - the options can be propagated between identical end points 
> together, so this combination is natural.

I think **#3** is the way to go. @aaron.ballman has agreed to this.

**#3a** vs **#3b** hasn't been decided upon yet:

> The bool (1) and the path (2) options can be passed through API layers 
> together in a struct. They can both be named generally 
> (preferStoringTempFilesInMemory, setTemporaryDirectory) or specifically 
> (storePreamblesInMemory, setPreambleStoragePath).

@aaron.ballman has put forth arguments in favor of specific names and separate 
API for different temporary directory uses. I have replied with 
counterarguments in favor of general temporary storage API.

In D139774#4081055 , @aaron.ballman 
wrote:

> modify `FileSystemOptions` to store an override for the temp directory

I have mentioned that `FileSystemOptions` is widely used and only class 
`ASTUnit` would need the temp directory (and possibly a flag whether to prefer 
RAM storage). The wide usage in itself is not considered a reason not to add 
data members.

`ASTUnit::FileSystemOpts` is used in several places in //ASTUnit.cpp//:

  FileSystemOpts = Clang->getFileSystemOpts();
  AST->FileSystemOpts = CI->getFileSystemOpts();
  AST->FileMgr = new FileManager(AST->FileSystemOpts, VFS);
  AST->FileSystemOpts = FileMgr->getFileSystemOpts();

I don't know whether propagating the new options to and from 
`CompilerInstance`, `CompilerInvocation` and `FileManager` is a good idea. As 
of now, only `ASTUnit::getMainBufferWithPrecompiledPreamble()` is going to use 
the new options.

> and updating APIs so that the temp directory can be specified by a new CIndex 
> constructor function.

Now that the temporary directory options are going to be propagated explicitly, 
I am no longer sure another constructor function is preferable to separate 
option setter(s). I don't see any use for temporary directory location in the 
definition of `clang_createIndex()`. And modifying the temporary directory 
location multiple times during program execution is no longer a problem: an 
updated location will be used for new preambles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

Now that a consensus has been reached that this revision is to be discarded, 
can we please resume the discussion of which alternative should be implemented 
in the replacement revision?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

One idea discussed in comments to the KDevelop merge request, which I haven't 
mentioned here is this: remove the preamble files immediately after creating 
and opening them. This is safe on Unix-like OSes, because every file that is 
still open will not actually be deleted but its directory entry cleared. It's 
an old Unix trick to mark (open) files as volatile. You could do this with any 
opened file (that you don't want to be able to close and then reopen) 
immediately after creating it in which case it'll get cleaned up even in case 
of a hard crash. However, my analysis of 
//clang/lib/Frontend/PrecompiledPreamble.cpp// shows that such unlinking isn't 
straightforward for the preamble files, e.g. because of 
`PrecompiledPreamble::getSize()`, which checks the file size at the preamble 
file path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4066593 , @dblaikie wrote:

>>> (1) seems OK-ish, I guess there's some question as to what the tradeoff is 
>>> for that option - does that blow out memory usage of the client/kdevelop? 
>>> But I guess it's probably fine.
>>
>> Do you think we should do one of the options for (2) or do you think (1) 
>> should be sufficient?
>
> If (1) is sufficient for KDevelop's needs and already implemented in some 
> form for clangd, if I understand correctly, that seems the cheapest/least 
> involved?

Not sufficient for all KDevelop users. Namely it doesn't work for low-RAM 
systems where /tmp is on disk to save RAM.

The bool (1) and the path (2) options can be passed through API layers together 
in a struct. They can both be named generally (preferStoringTempFilesInMemory, 
setTemporaryDirectory) or specifically (storePreamblesInMemory, 
setPreambleStoragePath).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4066519 , @dblaikie wrote:

> (1) seems OK-ish, I guess there's some question as to what the tradeoff is 
> for that option - does that blow out memory usage of the client/kdevelop? But 
> I guess it's probably fine.

At least one KDevelop user who has limited RAM and keeps /tmp on disk (not 
tmpfs) protested vehemently against unconditional storing of the temporary 
files in RAM. Several gigabytes of RAM can be valuable on some systems. Storing 
huge files in a temporary directory is definitely more flexible. But to users 
who keep /tmp on tmpfs, storing the preamble files in RAM is preferable, 
because memory is always freed right after a KDevelop crash, not on next start 
of the same KDevelop session (which can occur much later).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4065753 , @aaron.ballman 
wrote:

> I lean towards #2b over #2a due to wanting individual overrides rather than a 
> blanket override (e.g., we should be able to put preamble files in a 
> different location than we put, say, crash logs).

Crash logs don't really belong to a common temporary directory. Are they 
actually placed into a temporary directory erased on reboot? I'd prefer coarser 
categories, like temp dir erased-on-reboot and not-erased-on-reboot, similar to 
`void system_temp_directory(bool erasedOnReboot` in //Path.h//. This definitely 
makes more sense to the use case of removing these temporary files on next 
application start. Why would a libclang-using program care about categories of 
its internally-used temporary files?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

After a discussion under the corresponding KDevelop merge request, I can see 
4-6 alternative ways to address the temporary directory issue:

1. Add an option to store the //preamble-*.pch// files in RAM instead of /tmp 
and add a corresponding option in KDevelop configuration UI. This would work 
perfectly for me, provided I don't change my mind and decide to turn this 
option off, in which case I'll be back to square one.
2. Add an option to store the //preamble-*.pch// files in a custom directory 
instead of a general temporary directory. The option could be named generally 
(2a: overrideTemporaryDirectory) or specially (2b: setPreambleStoragePath). If 
the option is named generally, other temporary files created by libclang could 
be identified in the future and placed in the same directory without changing 
the API.
3. 1 and 2 - the options can be propagated between identical end points 
together, so this combination is natural.
4. The current patch. This is the easiest (already implemented) and most 
reliable (the temporary directory is definitely and completely overridden) 
approach. But there are thread safety concerns due to the introduction of 
global state.

If the 4th option is unacceptable, I lean towards the option 3a (1 and 2a), 
because the amount of implementation work should not be much greater than 1 
alone or 2a alone. If the 4th option is unacceptable, I suppose a separate 
review request based on the current LLVM main branch should be created and this 
one closed, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4047976 , @dblaikie wrote:

> It seemed like the places where kdevelop would want to suppress the temp dir 
> cleanup would be enumerable/more within kdevelop's control than instances of 
> libraries kdevelop is using wanting to create their own unexposed temp files. 
> But, yeah, would be a bunch of work to go and touch all those places. Ah well 
> *shrug*

That may be true. But a few small-size generated files slipping into the system 
temporary directory is not much of a problem. /tmp is cleared on shutdown after 
all. Causing bugs in a user executable run from KDevelop is much worse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a subscriber: milianw.
vedgy added a comment.

In D139774#4045361 , @dblaikie wrote:

> 1. Should clang be doing something better with these temp files anyway, no 
> matter the directory they go in? (setting them for cleanup at process exit or 
> the like - I think we have such a filesystem abstraction, maybe that only 
> works on Windows? I'm not sure)

That'd be great, but appears unfeasible on GNU/Linux in case of a crash: 
https://stackoverflow.com/questions/471344/guaranteed-file-deletion-upon-program-termination-c-c

> 2. If there isn't a better general way to avoid creating temp trash that's a 
> problem, I'd have thought that KDevelop/other tools would have issues with 
> other temp files too, and want a more general solution (I'd have thought 
> changing the process's temp directory, and changing it back for user process 
> launches, would be sufficient - so it can cleanup after itself for all files, 
> not just these particular clang-created ones)

I do plan to put other temporary files KDevelop generates in the same 
session-specific temporary directory and clean it on start. But modifying 
KDevelop's temporary directory environment variable has been rejected during 
code review for good reason. As the bug this review aims to fix 
 puts it:

> setting the environment variables is problematic, because they are inherited 
> by the IDE's code and all child processes it spawns (compiler, build system 
> and user-provided executables). The IDE must then remove the temporary 
> directory environment variable from each child process where it can cause 
> undesirable behavior.



In D139774#4045460 , @MaskRay wrote:

> libclang functions like `clang_reparseTranslationUnit_Impl` call 
> `clang/lib/Frontend/ASTUnit.cpp:1397` and build the preamble with 
> `/*StoreInMemory=*/false`.
> If `StoreInMemory` is configurable (true), then you can avoid the temporary 
> file `preamble-*.pch`.
>
> `clang/lib/Frontend/ASTUnit.cpp` is used by `clang/lib/Tooling/Tooling.cpp` 
> and libclang. Personally I think an unconditional `/*StoreInMemory=*/true` is 
> fine. (The concern is slightly memory usage increase).

This is a simple and promising solution. But maybe it should be configurable. 
The `StoreInMemory` option was introduced for the benefit of //clangd//, and 
//clangd// still stores the preambles on disk by default. See D39843 
. I'm pinging another KDevelop developer who 
may know better how this should work in KDevelop: @milianw


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4041308 , @aaron.ballman 
wrote:

> Is your concern essentially that someone will add a new use to put files in a 
> temp directory and not have access to this information from ASTUnit? Or do 
> you have other concerns in mind?
>
> We should certainly investigate whether there are other uses of temp files 
> from libclang as part of these changes. It's possible we'll find a situation 
> that makes my suggestion untenable because we don't have easy access to the 
> information we need.

The temporary directory could be used, now or in future, by some support code, 
which is used indirectly by libclang. I found the following uses potentially 
relevant to libclang:

- `Driver::GetTemporaryDirectory(StringRef Prefix)` calls 
`llvm::sys::fs::createUniqueDirectory(Prefix, Path);`
- `StandardInstrumentations` => `DotCfgChangeReporter` calls 
`sys::fs::createUniquePath("cfgdot-%%.dot", SV, true);`
- There are plenty of `createTemporaryFile()` uses in llvm and clang. Some of 
them are likely used by libclang.

I don't know how to efficiently check whether or not each of these indirect 
`system_temp_directory()` uses is in turn used by libclang. Even if they aren't 
know, they might be later, when libclang API grows.

On a different note, do you think overriding temporary directory path is useful 
only to libclang and not likely to be useful to any other LLVM API users?

>> Another issue is with the `FileSystemOptions` part: this class is widely 
>> used in Clang. So adding a member to it could adversely affect performance 
>> of unrelated code.
>
> It's good to keep an eye on that, but I'm not too worried about the overhead 
> from it (I don't see uses in AST nodes). We can keep an eye on 
> https://llvm-compile-time-tracker.com/ to see if there is a surprising 
> compile time increase though.

[in case we pursue this design approach, which I currently don't feel is right] 
Why not add a temporary path data member into `class ASTUnit` under the 
`FileSystemOptions FileSystemOpts` member, not inside it? So that other code is 
not burdened with unused struct member, and to be able to use a more suitable 
type, like `SmallString` for the temporary path buffer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4040705 , @aaron.ballman 
wrote:

> At a point where we have a `CIndexer` object, we eventually call 
> `ASTUnit::LoadFromCommandLine()` and `ASTUnit` has a member 
> `FileSystemOptions FileSystemOpts`, and `FileSystemOptions` has a 
> `std::string` for the working directory to use. I think we should store the 
> temp directory in here as well, and when 
> `ASTUnit::getMainBufferWithPrecompiledPreamble()` build the preamble, it can 
> pass that information along to `TempPCHFile` to avoid needing to ask the 
> system for the temp directory.

The path would have to be passed into several functions. 
`TempPCHFile::create()` would have to use `createUniqueFile()` instead of 
`createTemporaryFile()`. My greatest concern with this improved design is that 
libclang may use the temporary directory for other purposes - if not yet, then 
in the future. Another issue is with the `FileSystemOptions` part: this class 
is widely used in Clang. So adding a member to it could adversely affect 
performance of unrelated code.

> This does mean we store two copies of the string (one in `CIndexer` and one 
> in `FileSystemOptions`, but I think the improved ownership semantics for the 
> C API makes that duplication somewhat reasonable.

If LLVM does not have its own shared buffer type, a `std::shared_ptr` could be used instead of `std::string`. Or `SmallString` (not shared, 
but avoids allocations given a not too long overriding path).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4039975 , @aaron.ballman 
wrote:

> Oh that is a good point! Apologies for not noticing that earlier -- that 
> makes me wonder if a better approach here is to add a `std::string` to the 
> `CIndexer` class to represent the temp path to use.

I have suggested the possibility in this review:

> A copy of user-provided temporary directory path buffer can be stored in 
> class `CIndexer`. But this API in //llvm/Support/Path.h// would stay fragile.

So the question is whether the LLVM API or `CIndexer` should store the buffer.

The only possible caller of `system_temp_directory()` used by libclang is 
`llvm::sys::fs::createUniquePath()`. This helper function is called by 
`createUniqueEntity()`, which in turn is called by several other helper 
functions, all in //llvm/lib/Support/Path.cpp//. Here is a backtrace from 
KDevelop built against LLVM at this review's branch:

  #0 llvm::sys::path::system_temp_directory(bool, llvm::SmallVectorImpl&) 
(ErasedOnReboot=true, Result=...) at 
/home/Mint14_home/igor/Documents/C/LinuxProjects/external/llvm-project/llvm/lib/Support/Unix/Path.inc:1451
  #1 0x7fb372a55d60 in llvm::sys::fs::createUniquePath(llvm::Twine const&, 
llvm::SmallVectorImpl&, bool) (Model=, ResultPath=..., 
MakeAbsolute=) at 
/home/Mint14_home/igor/Documents/C/LinuxProjects/external/llvm-project/llvm/lib/Support/Path.cpp:811
  #2 0x7fb372a55eb7 in createUniqueEntity(llvm::Twine const&, int&, 
llvm::SmallVectorImpl&, bool, FSEntity, llvm::sys::fs::OpenFlags, 
unsigned int) (Model=..., ResultFD=@0x7fb36effc2d0: 0, ResultPath=..., 
MakeAbsolute=, Type=FS_File, Flags=llvm::sys::fs::OF_None, 
Mode=384) at 
/home/Mint14_home/igor/Documents/C/LinuxProjects/external/llvm-project/llvm/lib/Support/Path.cpp:181
  #3 0x7fb372a5623b in llvm::sys::fs::createTemporaryFile 
(Flags=llvm::sys::fs::OF_None, Type=FS_File, ResultPath=..., 
ResultFD=@0x7fb36effc2d0: 0, Model=...) at 
/home/Mint14_home/igor/Documents/C/LinuxProjects/external/llvm-project/llvm/include/llvm/ADT/Twine.h:233
  #4 llvm::sys::fs::createTemporaryFile(llvm::Twine const&, llvm::StringRef, 
int&, llvm::SmallVectorImpl&, FSEntity, llvm::sys::fs::OpenFlags) 
(Prefix=, Suffix=..., ResultFD=@0x7fb36effc2d0: 0, 
ResultPath=..., Type=Type@entry=FS_File, Flags=llvm::sys::fs::OF_None) at 
/home/Mint14_home/igor/Documents/C/LinuxProjects/external/llvm-project/llvm/lib/Support/Path.cpp:865
  #5 0x7fb372a56451 in llvm::sys::fs::createTemporaryFile(llvm::Twine 
const&, llvm::StringRef, int&, llvm::SmallVectorImpl&, 
llvm::sys::fs::OpenFlags) (Prefix=, Suffix=..., 
ResultFD=, ResultPath=, Flags=) at 
/home/Mint14_home/igor/Documents/C/LinuxProjects/external/llvm-project/llvm/lib/Support/Path.cpp:873
  #6 0x7fb3717cc1b7 in (anonymous namespace)::TempPCHFile::create () at 
/home/Mint14_home/igor/Documents/C/LinuxProjects/external/llvm-project/llvm/include/llvm/ADT/Twine.h:233
  #7 clang::PrecompiledPreamble::Build(clang::CompilerInvocation const&, 
llvm::MemoryBuffer const*, clang::PreambleBounds, clang::DiagnosticsEngine&, 
llvm::IntrusiveRefCntPtr, 
std::shared_ptr, bool, 
clang::PreambleCallbacks&) (Invocation=..., MainFileBuffer=0x7fb35000a360, 
Bounds=Bounds@entry=..., Diagnostics=..., VFS=..., 
PCHContainerOps=std::shared_ptr (use count 5, 
weak count 0) = {...}, StoreInMemory=false, Callbacks=...) at 
/home/igor/Documents/C/LinuxProjects/external/llvm-project/clang/lib/Frontend/PrecompiledPreamble.cpp:421
  #8 0x7fb3717234a8 in 
clang::ASTUnit::getMainBufferWithPrecompiledPreamble(std::shared_ptr,
 clang::CompilerInvocation&, llvm::IntrusiveRefCntPtr, 
bool, unsigned int) (this=0x7fb3505c44d0, 
PCHContainerOps=std::shared_ptr (use count 5, 
weak count 0) = {...}, PreambleInvocationIn=..., VFS=..., 
AllowRebuild=, MaxLines=0) at 
/usr/include/c++/12.2.0/bits/unique_ptr.h:191
  #9 0x7fb371729bd6 in 
clang::ASTUnit::LoadFromCompilerInvocation(std::shared_ptr,
 unsigned int, llvm::IntrusiveRefCntPtr) 
(this=0x7fb3505c44d0, 
PCHContainerOps=std::shared_ptr (use count 5, 
weak count 0) = {...}, PrecompilePreambleAfterNParses=, VFS=...) 
at 
/home/igor/Documents/C/LinuxProjects/external/llvm-project/clang/lib/Frontend/ASTUnit.cpp:1685
  #10 0x7fb37172e359 in clang::ASTUnit::LoadFromCommandLine(char const**, 
char const**, std::shared_ptr, 
llvm::IntrusiveRefCntPtr, llvm::StringRef, bool, 
clang::CaptureDiagsKind, 
llvm::ArrayRef, std::allocator >, llvm::MemoryBuffer*> >, bool, 
unsigned int, clang::TranslationUnitKind, bool, bool, bool, 
clang::SkipFunctionBodiesScope, bool, bool, bool, bool, 
llvm::Optional, std::unique_ptr >*, 
llvm::IntrusiveRefCntPtr) (ArgBegin=, 
ArgEnd=, 
PCHContainerOps=std::shared_ptr (empty) = {...}, 
Diags=..., ResourceFilesPath=..., OnlyLocalDecls=false, 
CaptureDiagnostics=clang::CaptureDiagsKind::All, RemappedFiles=..., 
RemappedFilesKeepOriginalName=true, PrecompilePreambleAfterNParses=1, 
TUKind=clang::TU_Complete, 

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4036496 , @aaron.ballman 
wrote:

> Given that we already have other setters to set global state, I'm less 
> concerned about adding another one. I hadn't realized we already introduced 
> the dangers here. But we should document the expectation that the call be 
> made before creating the index.

There is a difference: `clang_CXIndex_setGlobalOptions()` and 
`clang_CXIndex_setInvocationEmissionPathOption()` take a `CXIndex` argument and 
thus can only be called **after** creating an index. So requiring to call 
`clang_overrideTemporaryDirectory()` before creating an index, plus one more 
time with `nullptr` argument after disposing of the index, wouldn't be 
particularly consistent with other setters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy marked an inline comment as not done.
vedgy added a comment.

In D139774#4036496 , @aaron.ballman 
wrote:

> In terms of the C API, I think it'd make more sense to name in terms of 
> "override" rather than "set", but I don't feel as strongly about it given the 
> other setters. In terms of the C++ file system API, I think "override" makes 
> the most sense though (we don't have setters to follow the naming convention 
> for) because some systems do allow you to set the system directory.

Let's keep the naming in C and C++ APIs consistent: 
`clang_overrideTemporaryDirectory()` and 
`override_system_temp_directory_erased_on_reboot()`.

> In terms of memory ownership, WDYT of requiring the caller to handle this? 
> e.g., calling `set_system_temp_directory_erased_on_reboot` will `strdup` a 
> nonnull pointer and `free` the stored pointer when given nullptr.

I like this idea. libclang-user code would become easier to use than it is now 
(though less easy compared to libclang managing memory itself). The libclang 
API documentation can require overriding the temp directory before creating an 
index and un-overriding it with `nullptr` after calling `clang_disposeIndex()`.
Now in order to make this libclang API harder to misuse, I lean towards passing 
the temporary directory in `clang_createIndexWithTempDir()` and letting 
`clang_disposeIndex()` handle the un-overriding (call 
`override_system_temp_directory_erased_on_reboot(nullptr)`) automatically. 
Makes sense? I feel that the `clang_createIndexWithTempDir()` name could be 
improved, but don't know how...

What memory [de]allocation method should 
`override_system_temp_directory_erased_on_reboot()` use? `new[]` and 
`delete[]`? Or should `strdup()` from POSIX be used because it is defined as 
`_strdup` on Windows? (along with standard `free()`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy marked an inline comment as done and 2 inline comments as not done.
vedgy added inline comments.



Comment at: llvm/include/llvm/Support/Path.h:423
+/// tempDirUtf8 pointer previously passed to this function.
+void set_system_temp_directory_erased_on_reboot(const char *tempDirUtf8);
+

vedgy wrote:
> aaron.ballman wrote:
> > Err, I'm not super excited about this new API. For starters, it's not 
> > setting the system temp directory at all (it doesn't make any modifications 
> > to the host system); instead it overrides the system temp directory. But 
> > also, this is pretty fragile due to allowing the user to override the temp 
> > directory after the compiler has already queried for the system temp 
> > directory, so now you get files in two different places. Further, it's 
> > fragile because the caller is responsible for keeping that pointer valid 
> > for the lifetime of the program. Finally, we don't allow you to override 
> > any other system directory that you can query (like the home directory).
> >  it's not setting the system temp directory at all (it doesn't make any 
> > modifications to the host system); instead it overrides the system temp 
> > directory.
> Rename to `override_system_temp_directory_erased_on_reboot()`?
> 
> > this is pretty fragile due to allowing the user to override the temp 
> > directory after the compiler has already queried for the system temp 
> > directory, so now you get files in two different places.
> Unfortunately, I don't know how to prevent this. In practice calling 
> `clang_setTemporaryDirectory()` before `clang_createIndex()` works perfectly 
> well in KDevelop: the //preamble-*.pch// files are created later and never 
> end up in the default temporary directory ///tmp//. The same issue applies to 
> the current querying of the environment variable values repeatedly: the user 
> can set environment variables at any time.
> 
> >  it's fragile because the caller is responsible for keeping that pointer 
> > valid for the lifetime of the program.
> I'd love to duplicate the temporary directory path buffer in Path.cpp. The 
> API would become much easier to use. But then the buffer would have to be 
> destroyed when libclang is unloaded or on program shutdown, which is 
> forbidden by LLVM Coding Standards: //Do not use Static Constructors//. That 
> is, I think the following code in Path.cpp is not acceptable:
> ```
> static SmallVectorImpl ()
> {
>   static SmallVector value;
>   return value;
> }
> ```
> 
> > we don't allow you to override any other system directory that you can 
> > query (like the home directory).
> Well, one has to start somewhere :) Seriously, overriding directories should 
> be allowed only if it has a practical use case. Not just because overriding 
> others is allowed.
A copy of user-provided temporary directory path buffer can be stored in `class 
CIndexer`. But this API in //llvm/Support/Path.h// would stay fragile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4025356 , @vedgy wrote:

> `clang_disposeIndex()` would have to un-override the temporary directory 
> then. I'll have to check whether this un-overriding happens too early (before 
> all //preamble-*.pch// files are removed).

Checked by calling `clang_setTemporaryDirectory(nullptr);` right after 
`clang_disposeIndex(m_index);`. Works correctly in KDevelop: all 
//preamble-*.pch// files are removed from the overriding temporary directory on 
exit. So this alternative API is viable.




Comment at: clang/include/clang-c/Index.h:78-79
+ * this function in order to reset the temporary directory to the default value
+ * from the environment. Such a resetting should be done before deleting a
+ * tempDirUtf8 pointer previously passed to this function.
+ */

vedgy wrote:
> aaron.ballman wrote:
> > Should we mention anything about relative vs absolute path requirements? 
> > Separators? 
> On Windows separators are converted automatically. I suppose we don't need to 
> warn users not to pass Windows-style separators on Unix.
> 
> On Windows this function handles both relative and absolute paths. On Unix 
> the existing implementation doesn't check whether the path is absolute or 
> relative. Perhaps it assumes that the path in the environment variable is 
> always absolute. Or absolute vs relative doesn't matter. I'll check what 
> happens if a relative path is passed.
Tested passing a relative path on GNU/Linux: works as expected. libclang stores 
and removes the //preamble-*.pch// in the same absolute path as 
[QDir::absolutePath()](https://doc.qt.io/qt-5/qdir.html#absolutePath) returns 
in KDevelop. Since passing any reasonably formatted path to this function 
works, I suppose mentioning the path requirements in the documentation is 
unnecessary.

While testing, I noticed another requirement that **should** be mentioned in 
this documentation, as well as `set_system_temp_directory_erased_on_reboot`'s 
documentation: the temporary directory must exist, because Clang doesn't create 
it. I couldn't find where Clang writes the preamble files when the temporary 
directory does not exist. Perhaps it doesn't write them to disk at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy added a comment.

In D139774#4023940 , @aaron.ballman 
wrote:

> This feels like a configuration property of the libclang execution -- so it'd 
> be nice to require it to be set up from `clang_createIndex()` (IIRC that's 
> the entrypoint for CIndex functionality, but I'm not 100% sure), rather than 
> an API that the user can call repeatedly.

In order to keep backward compatibility, this would require introducing another 
`createIndex` function, e.g. `clang_createIndexWithTempDir()`. 
`clang_disposeIndex()` would have to un-override the temporary directory then. 
I'll have to check whether this un-overriding happens too early (before all 
//preamble-*.pch// files are removed). But notice that separate setup functions 
already exist: `clang_CXIndex_setGlobalOptions()`, 
`clang_CXIndex_setInvocationEmissionPathOption()`. The documentation for 
`clang_setTemporaryDirectory()` can recommend calling it before 
`clang_createIndex()`.
Either alternative works for KDevelop, because it calls `clang_createIndex()` 
once.




Comment at: clang/include/clang-c/Index.h:78-79
+ * this function in order to reset the temporary directory to the default value
+ * from the environment. Such a resetting should be done before deleting a
+ * tempDirUtf8 pointer previously passed to this function.
+ */

aaron.ballman wrote:
> Should we mention anything about relative vs absolute path requirements? 
> Separators? 
On Windows separators are converted automatically. I suppose we don't need to 
warn users not to pass Windows-style separators on Unix.

On Windows this function handles both relative and absolute paths. On Unix the 
existing implementation doesn't check whether the path is absolute or relative. 
Perhaps it assumes that the path in the environment variable is always 
absolute. Or absolute vs relative doesn't matter. I'll check what happens if a 
relative path is passed.



Comment at: llvm/include/llvm/Support/Path.h:423
+/// tempDirUtf8 pointer previously passed to this function.
+void set_system_temp_directory_erased_on_reboot(const char *tempDirUtf8);
+

aaron.ballman wrote:
> Err, I'm not super excited about this new API. For starters, it's not setting 
> the system temp directory at all (it doesn't make any modifications to the 
> host system); instead it overrides the system temp directory. But also, this 
> is pretty fragile due to allowing the user to override the temp directory 
> after the compiler has already queried for the system temp directory, so now 
> you get files in two different places. Further, it's fragile because the 
> caller is responsible for keeping that pointer valid for the lifetime of the 
> program. Finally, we don't allow you to override any other system directory 
> that you can query (like the home directory).
>  it's not setting the system temp directory at all (it doesn't make any 
> modifications to the host system); instead it overrides the system temp 
> directory.
Rename to `override_system_temp_directory_erased_on_reboot()`?

> this is pretty fragile due to allowing the user to override the temp 
> directory after the compiler has already queried for the system temp 
> directory, so now you get files in two different places.
Unfortunately, I don't know how to prevent this. In practice calling 
`clang_setTemporaryDirectory()` before `clang_createIndex()` works perfectly 
well in KDevelop: the //preamble-*.pch// files are created later and never end 
up in the default temporary directory ///tmp//. The same issue applies to the 
current querying of the environment variable values repeatedly: the user can 
set environment variables at any time.

>  it's fragile because the caller is responsible for keeping that pointer 
> valid for the lifetime of the program.
I'd love to duplicate the temporary directory path buffer in Path.cpp. The API 
would become much easier to use. But then the buffer would have to be destroyed 
when libclang is unloaded or on program shutdown, which is forbidden by LLVM 
Coding Standards: //Do not use Static Constructors//. That is, I think the 
following code in Path.cpp is not acceptable:
```
static SmallVectorImpl ()
{
  static SmallVector value;
  return value;
}
```

> we don't allow you to override any other system directory that you can query 
> (like the home directory).
Well, one has to start somewhere :) Seriously, overriding directories should be 
allowed only if it has a practical use case. Not just because overriding others 
is allowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy updated this revision to Diff 481913.
vedgy edited the summary of this revision.
vedgy added a comment.

Extract identical code from the two Path.inc files into Path.cpp

One of the Path.inc files is #include-d into this Path.cpp file and nowhere 
else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139774

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/libclang.map
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Path.cpp
  llvm/lib/Support/Unix/Path.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -591,6 +591,26 @@
   EXPECT_TRUE(!TempDir.empty());
 }
 
+TEST(Support, SetTempDirectory) {
+  SmallString<64> DefaultTempDir;
+  path::system_temp_directory(true, DefaultTempDir);
+  EXPECT_TRUE(!DefaultTempDir.empty());
+
+  auto CustomTempDir = DefaultTempDir;
+  path::append(CustomTempDir, "/llvm/test_temp_dir");
+  path::native(CustomTempDir);
+  path::set_system_temp_directory_erased_on_reboot(CustomTempDir.c_str());
+
+  SmallString<64> TempDir;
+  path::system_temp_directory(true, TempDir);
+  EXPECT_EQ(CustomTempDir, TempDir);
+
+  path::set_system_temp_directory_erased_on_reboot(nullptr);
+  TempDir.clear();
+  path::system_temp_directory(true, TempDir);
+  EXPECT_EQ(DefaultTempDir, TempDir);
+}
+
 #ifdef _WIN32
 static std::string path2regex(std::string Path) {
   size_t Pos = 0;
Index: llvm/lib/Support/Windows/Path.inc
===
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -1472,11 +1472,16 @@
   (void)ErasedOnReboot;
   Result.clear();
 
+  if (tempDirErasedOnRebootUtf8) {
+const auto len = strlen(tempDirErasedOnRebootUtf8);
+Result.append(tempDirErasedOnRebootUtf8, tempDirErasedOnRebootUtf8 + len);
+  }
+
   // Check whether the temporary directory is specified by an environment var.
   // This matches GetTempPath logic to some degree. GetTempPath is not used
   // directly as it cannot handle evn var longer than 130 chars on Windows 7
   // (fixed on Windows 8).
-  if (getTempDirEnvVar(Result)) {
+  if (!Result.empty() || getTempDirEnvVar(Result)) {
 assert(!Result.empty() && "Unexpected empty path");
 native(Result); // Some Unix-like shells use Unix path separator in $TMP.
 fs::make_absolute(Result); // Make it absolute if not already.
Index: llvm/lib/Support/Unix/Path.inc
===
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1451,8 +1451,11 @@
   Result.clear();
 
   if (ErasedOnReboot) {
+const char *RequestedDir = tempDirErasedOnRebootUtf8;
 // There is no env variable for the cache directory.
-if (const char *RequestedDir = getEnvTempDir()) {
+if (!RequestedDir)
+  RequestedDir = getEnvTempDir();
+if (RequestedDir) {
   Result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
   return;
 }
Index: llvm/lib/Support/Path.cpp
===
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -780,6 +780,12 @@
   return true;
 }
 
+static const char *tempDirErasedOnRebootUtf8 = nullptr;
+
+void set_system_temp_directory_erased_on_reboot(const char *tempDirUtf8) {
+  tempDirErasedOnRebootUtf8 = tempDirUtf8;
+}
+
 } // end namespace path
 
 namespace fs {
Index: llvm/include/llvm/Support/Path.h
===
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ -412,6 +412,16 @@
 /// @param result Holds the resulting path name.
 void system_temp_directory(bool erasedOnReboot, SmallVectorImpl );
 
+/// Override the temporary directory path returned by
+/// system_temp_directory(true, result).
+///
+/// @param tempDirUtf8 UTF-8-encoded path to the desired temporary directory.
+/// The pointer is owned by the caller and must be always valid. Pass nullptr to
+/// this function in order to reset the temporary directory to the default value
+/// from the environment. Such a resetting should be done before deleting a
+/// tempDirUtf8 pointer previously passed to this function.
+void set_system_temp_directory_erased_on_reboot(const char *tempDirUtf8);
+
 /// Get the user's home directory.
 ///
 /// @param result Holds the resulting path name.
Index: clang/tools/libclang/libclang.map
===
--- clang/tools/libclang/libclang.map
+++ clang/tools/libclang/libclang.map
@@ -407,6 +407,7 @@
 
 LLVM_16 {
   global:
+clang_setTemporaryDirectory;
 clang_getUnqualifiedType;
 

[PATCH] D139774: [libclang] Add API to set temporary directory location

[Igor Kushnir via Phabricator via cfe-commits]

vedgy created this revision.
vedgy added a reviewer: aaron.ballman.
Herald added subscribers: arphaman, hiraditya.
Herald added a project: All.
vedgy requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Fixes: https://github.com/llvm/llvm-project/issues/51847


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139774

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang-c/Index.h
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/libclang.map
  llvm/include/llvm/Support/Path.h
  llvm/lib/Support/Unix/Path.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/unittests/Support/Path.cpp

Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -591,6 +591,26 @@
   EXPECT_TRUE(!TempDir.empty());
 }
 
+TEST(Support, SetTempDirectory) {
+  SmallString<64> DefaultTempDir;
+  path::system_temp_directory(true, DefaultTempDir);
+  EXPECT_TRUE(!DefaultTempDir.empty());
+
+  auto CustomTempDir = DefaultTempDir;
+  path::append(CustomTempDir, "/llvm/test_temp_dir");
+  path::native(CustomTempDir);
+  path::set_system_temp_directory_erased_on_reboot(CustomTempDir.c_str());
+
+  SmallString<64> TempDir;
+  path::system_temp_directory(true, TempDir);
+  EXPECT_EQ(CustomTempDir, TempDir);
+
+  path::set_system_temp_directory_erased_on_reboot(nullptr);
+  TempDir.clear();
+  path::system_temp_directory(true, TempDir);
+  EXPECT_EQ(DefaultTempDir, TempDir);
+}
+
 #ifdef _WIN32
 static std::string path2regex(std::string Path) {
   size_t Pos = 0;
Index: llvm/lib/Support/Windows/Path.inc
===
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -1468,15 +1468,22 @@
   return false;
 }
 
+static const char *tempDirErasedOnRebootUtf8 = nullptr;
+
 void system_temp_directory(bool ErasedOnReboot, SmallVectorImpl ) {
   (void)ErasedOnReboot;
   Result.clear();
 
+  if (tempDirErasedOnRebootUtf8) {
+const auto len = strlen(tempDirErasedOnRebootUtf8);
+Result.append(tempDirErasedOnRebootUtf8, tempDirErasedOnRebootUtf8 + len);
+  }
+
   // Check whether the temporary directory is specified by an environment var.
   // This matches GetTempPath logic to some degree. GetTempPath is not used
   // directly as it cannot handle evn var longer than 130 chars on Windows 7
   // (fixed on Windows 8).
-  if (getTempDirEnvVar(Result)) {
+  if (!Result.empty() || getTempDirEnvVar(Result)) {
 assert(!Result.empty() && "Unexpected empty path");
 native(Result); // Some Unix-like shells use Unix path separator in $TMP.
 fs::make_absolute(Result); // Make it absolute if not already.
@@ -1488,6 +1495,10 @@
   Result.append(DefaultResult, DefaultResult + strlen(DefaultResult));
   llvm::sys::path::make_preferred(Result);
 }
+
+void set_system_temp_directory_erased_on_reboot(const char *tempDirUtf8) {
+  tempDirErasedOnRebootUtf8 = tempDirUtf8;
+}
 } // end namespace path
 
 namespace windows {
Index: llvm/lib/Support/Unix/Path.inc
===
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1447,12 +1447,17 @@
   return "/var/tmp";
 }
 
+static const char *tempDirErasedOnRebootUtf8 = nullptr;
+
 void system_temp_directory(bool ErasedOnReboot, SmallVectorImpl ) {
   Result.clear();
 
   if (ErasedOnReboot) {
+const char *RequestedDir = tempDirErasedOnRebootUtf8;
 // There is no env variable for the cache directory.
-if (const char *RequestedDir = getEnvTempDir()) {
+if (!RequestedDir)
+  RequestedDir = getEnvTempDir();
+if (RequestedDir) {
   Result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
   return;
 }
@@ -1465,6 +1470,10 @@
   Result.append(RequestedDir, RequestedDir + strlen(RequestedDir));
 }
 
+void set_system_temp_directory_erased_on_reboot(const char *tempDirUtf8) {
+  tempDirErasedOnRebootUtf8 = tempDirUtf8;
+}
+
 } // end namespace path
 
 namespace fs {
Index: llvm/include/llvm/Support/Path.h
===
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ -412,6 +412,16 @@
 /// @param result Holds the resulting path name.
 void system_temp_directory(bool erasedOnReboot, SmallVectorImpl );
 
+/// Override the temporary directory path returned by
+/// system_temp_directory(true, result).
+///
+/// @param tempDirUtf8 UTF-8-encoded path to the desired temporary directory.
+/// The pointer is owned by the caller and must be always valid. Pass nullptr to
+/// this function in order to reset the temporary directory to the default value
+/// from the environment. Such a resetting should be done before deleting a
+/// tempDirUtf8 pointer previously passed to this function.
+void