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

In D132749#3752233 <https://reviews.llvm.org/D132749#3752233>, @diseraluca 
wrote:

> I found a precedence on the old mailing list, from 2018 
> (https://lists.llvm.org/pipermail/cfe-dev/2018-February/056874.html), for a 
> similar request, where it seemed it wouldn't be disruptive to expose it to 
> libclang.

Yup! FWIW, we will add things to libclang as people say they found a need for 
them (we do similar for AST matchers). We mostly just want to avoid trying to 
expose all of the APIs through a C interface, as that causes significant 
maintenance burden given how rapidly our APIs change.

> This is my first contribution to llvm-project, so I apologize if I missed 
> some requirement or pushed a patch that is incorrect. 
> I'm obviously open to spend more time on this patch if there is something 
> that should be modified.

Welcome to the community!

> Originally the patch contained the addition of `clang_getNonReferenceType` 
> too, as that is something else that we would like to have access to.

That seems perfectly reasonable to me as well.

> I avoided including it into this patch because:
>
> - As I understand it, having both in one patch would not comply with the 
> isolated changes requirement in 
> https://www.llvm.org/docs/Contributing.html#how-to-submit-a-patch.
> - I could not find any precedence for requesting that specific interface and 
> am unsure if the correct etiquette requires that a discussion is opened 
> BEFORE pushing this kind of patch.
> - First pushing a smaller patch would allow me to get accustomed to the 
> contributing workflow and understand what I might have done incorrectly, so 
> as to push an higher quality patch.
>
> Any clarification on if a patch that exposes `getNonReferencedType` requires 
> a discussion to be opened before being pushed for acceptance and review is 
> appreciated.

Feel free to put up a new review for that and add me as a reviewer, I'm happy 
to look at it. It should be a separate patch because we like to keep patches 
logically separate in case we need to revert something (no sense in losing all 
the good changes along with the bad by sticking them all in one patch). It's 
also far easier to review a patch without a lot of moving parts to it.

In terms of this patch, it looks good to me, but can you add a release note for 
the improvement? (We have a section for `libclang` that this should go under.) 
Once you post the patch with the release note, I'm happy to land this on your 
behalf (assuming precommit CI doesn't find any surprises I missed). What name 
and email address would you like used for patch attribution?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132749

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

Reply via email to