[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] D143418: [libclang] Add API to override preamble storage path

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

vedgy wrote:
> 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.
Thank you, I've landed both of them.


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;
+  /**

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] D143418: [libclang] Add API to override preamble storage path

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

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.


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:
> 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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D143418#4175887 , @vedgy wrote:

> 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.

Ah, thank you for the explanation!

>>> 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.

Yeah, we could make it thread safe, but I still don't think setters are a good 
design approach. To me, these global options should be ones that are set when 
creating the index and be immutable from there on. (This mirrors how language 
options work in Clang itself -- we have a ton of language options, but they get 
set up by the compiler frontend and are (generally) immutable from there on. 
When we need to allow for different options, the interface accepts a 
`LangOptions` object, as in: 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Analysis/CFG.h#L1081)

>> 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?

Yes, thank you! #3/#4 are really the biggest advantages, to me. By passing in 
the options explicitly to the TU instead of having setters, we reduce the 
complexity of the interface because it no longer becomes possible for an option 
to change mid-parse. This in turn makes testing far easier because we don't 
have to come up with test coverage for "what if the option was X and it got 
switched to Y before calling this function" kind of situations.

> 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()`.

I think that's a good approach. You are a consumer of libclang, so having your 
perspective about what makes the interface easier for you to use is also really 
helpful in figuring out a good design. Thank you for being willing to share 
your experiences with using libclang on KDevelop!




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

[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] 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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D143418#4175381 , @vedgy wrote:

> 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.

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 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. 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?


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D143418#4175188 , @vedgy wrote:

> Thank you for the quick build fix.

You're welcome!

> 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.

https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/HandleLLVMOptions.cmake#L730


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D143418#4175128 , @vedgy wrote:

> 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.

It's a two-stage build. The first stage passed, but the second stage is built 
with `-Werror`, which is why it fails. I addressed the issue in 
df8f8f76207df40dca11c9c0c2328d6b3dfba9ca 
, so no 
need for you to worry about making a patch. I went with `{}` to perform the 
zero initialization instead of `memset`, but either should work fine.


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] D143418: [libclang] Add API to override preamble storage path

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D143418#4174521 , @vedgy wrote:

> 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.

Thanks for the explanation, I'm happy to land on your behalf (esp if you don't 
plan to make many more changes in the future). I landed the changes in 
cc929590ad305f0d068709c7c7999f5fc6118dc9 
. I'm not 
of much help with arcanist as I've never actually used it, but I think you're 
correct about only needing to do `arc land`. In terms of babysitting the build 
farm, there's not too much to it -- most bots are configured to send an email 
to the folks on the blamelist when an issue is found. It's mostly a matter of 
being around to check your email, but you can manually watch from 
https://lab.llvm.org/buildbot/#/builders if you enjoy that sort of thing.


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

[Aaron Ballman via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcc929590ad30: [libclang] Add API to override preamble 
storage path (authored by vedgy, committed by aaron.ballman).

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 

[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] D143418: [libclang] Add API to override preamble storage path

[Aaron Ballman via Phabricator via cfe-commits]

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

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.


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 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] D143418: [libclang] Add API to override preamble storage path

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

The changes look correct to me, but precommit CI isn't able to apply the patch 
cleanly to test. Can you rebase the patch so it applies so we can smoke test it 
before acceptance?


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 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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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:
> > > 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 

[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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

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 `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 

[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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

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()`).


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 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] D143418: [libclang] Add API to override preamble storage path

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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");
+

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 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 

[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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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");
+

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 C++.

Familiarity for those who are used to dealing with existing C APIs that behave 
this way (like Win32 APIs), but that can probably be argued either way. My 
guess is that all libclang users are having to work 

[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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

vedgy wrote:
> 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.
SGTM, thanks for the explanation



Comment at: clang/include/clang-c/Index.h:353
+ */
+CINDEX_LINKAGE unsigned clang_getDefaultGlobalOptions();
+

Don't want this to be a K C interface in pre-C2x modes.



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

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?



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

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.



Comment at: clang/tools/libclang/CIndex.cpp:3691
 
+unsigned clang_getDefaultGlobalOptions() {
+  unsigned GlobalOptions = CXGlobalOpt_None;





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,

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 (options->Size >= offsetof(CXIndexOptions, FieldWeCareAbout))
  do_something(options->FieldWeCareAbout);
```


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: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();
+