SourceLocation refactoring review

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

SourceLocation refactoring review

Fangrui Song via cfe-dev
Hi folks,

I have series of patches which refactor SourceLocation uses to reduce the number of places where the raw (integer) representation of SourceLocation objects is accessed directly:
* https://reviews.llvm.org/D69840 - [Basic] Make SourceLocation usable as key in hash maps, NFCI
* https://reviews.llvm.org/D69844 - [Basic] Integrate SourceLocation and SourceRange with FoldingSet, NFCI
* https://reviews.llvm.org/D69903 - [Basic] Introduce PODSourceLocation, NFCI

When Adrian Prantl was reviewing the first patch he asked me about my motivation for these patches, and I replied that the goal is to be able to configure the bit width of the SourceLocation representation (32 or 64 bits), this proposal had been discussed on the mailing list: http://lists.llvm.org/pipermail/cfe-dev/2019-October/063515.html

Apparently the statement about my motivation made the reviewers (not just Adrian) very reluctant to continuing the review, probably because changing the SourceLocation representation is a major change in a ubiquitous class.

Nevertheless I would like to emphasize the patch series itself does not introduce any changes to the representation of SourceLocation-s. The change is just a refactoring (which in my opinion makes the code cleaner and better in terms of expressing the intent). We don't actually need consensus on the representation change to apply the patch series because it is an independent change. So I am kindly requesting the Clang code owners to review the patches.

--
Regards,
   Mikhail Maltsev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceLocation refactoring review

Fangrui Song via cfe-dev
On Fri, 24 Apr 2020 at 01:55, Mikhail Maltsev via cfe-dev
<[hidden email]> wrote:

>
> Hi folks,
>
> I have series of patches which refactor SourceLocation uses to reduce the number of places where the raw (integer) representation of SourceLocation objects is accessed directly:
> * https://reviews.llvm.org/D69840 - [Basic] Make SourceLocation usable as key in hash maps, NFCI
> * https://reviews.llvm.org/D69844 - [Basic] Integrate SourceLocation and SourceRange with FoldingSet, NFCI
> * https://reviews.llvm.org/D69903 - [Basic] Introduce PODSourceLocation, NFCI
>
> When Adrian Prantl was reviewing the first patch he asked me about my motivation for these patches, and I replied that the goal is to be able to configure the bit width of the SourceLocation representation (32 or 64 bits), this proposal had been discussed on the mailing list: http://lists.llvm.org/pipermail/cfe-dev/2019-October/063515.html
>
> Apparently the statement about my motivation made the reviewers (not just Adrian) very reluctant to continuing the review, probably because changing the SourceLocation representation is a major change in a ubiquitous class.
>
> Nevertheless I would like to emphasize the patch series itself does not introduce any changes to the representation of SourceLocation-s. The change is just a refactoring (which in my opinion makes the code cleaner and better in terms of expressing the intent). We don't actually need consensus on the representation change to apply the patch series because it is an independent change. So I am kindly requesting the Clang code owners to review the patches.

Does this impact clang tools extra? I've noticed an annoying "feature" there:
clang-tidy raises errors for rules violation caused by pre-processor
macros expansion defined in "system headers" expanded in user code.
This is quite annoying to me, it avoids me to deploy clang-tidy checks
(Root errors come from macros in Qt, boost, Google test, ... ).

Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: SourceLocation refactoring review

Fangrui Song via cfe-dev
From: Christian Gagneraud <[hidden email]>
Sent: Thursday, April 23, 2020 15:10

> Does this impact clang tools extra? I've noticed an annoying "feature" there:
> clang-tidy raises errors for rules violation caused by pre-processor
> macros expansion defined in "system headers" expanded in user code.
> This is quite annoying to me, it avoids me to deploy clang-tidy checks
> (Root errors come from macros in Qt, boost, Google test, ... ).

The refactoring does affect clang tools extra, but just as any refactoring it is a
non-functional change, i.e. it is not expected to change the behavior.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev