[PATCH] D132749: Expose QualType::getUnqualifiedType in libclang

Fri Aug 26 11:53:26 PDT 2022

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