Endilll wrote:
@davidstone Do you plan to get back to this PR?
https://github.com/llvm/llvm-project/pull/93493
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/ChuanqiXu9 approved this pull request.
I feel good with this too.
But if later you have similar multiple consecutive large NFC patches like this,
I'll recommend you send the PR as stacked PR and then we can land the
continuously. It'll help the downstream project slightly.
https://github.com/AaronBallman approved this pull request.
I looked through the review and I think this is generally an improvement. We
may find a need to relax some `const` later, but I did not spot any obvious
concerns.
That said, I'd like @ChuanqiXu9 to give the final sign-off on this as m
https://github.com/AaronBallman edited
https://github.com/llvm/llvm-project/pull/93493
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ChuanqiXu9 wrote:
BTW, for the series patches, it may be better to use stacked PR (we use stacked
PR by the method in https://llvm.org/docs/GitHub.html) if you have following
patches (and it looks like you have a lot).
Also, as a downstream vendor, I hope we can land the (large, NFC) patch ser
@@ -817,11 +817,11 @@ class alignas(8) Decl {
"owned local decl but no local module storage");
return reinterpret_cast(this)[-1];
}
- void setLocalOwningModule(Module *M) {
+ void setLocalOwningModule(const Module *M) {
davidstone wrote:
Hmm
Endilll wrote:
> > `const_cast` here is a relatively recent addition, and I checked out with
> > @erichkeane that the use case (pack expansion) is legit. According to the
> > approach you're suggesting, the one who wrote this `const_cast` should
> > instead refactor the MLTAL to use `llvm::Mut
erichkeane wrote:
> One suggestion, if you are going to go about this problem systematically, try
> to tackle the hardest problems first.
>
> For example: Try to make `MultiLevelTemplateArgumentList` const correct. I
> would like other suggestions as well.
Right, yeah. Unfortunately const-co
mizvekov wrote:
> `const_cast` here is a relatively recent addition, and I checked out with
> @erichkeane that the use case (pack expansion) is legit. According to the
> approach you're suggesting, the one who wrote this `const_cast` should
> instead refactor the MLTAL to use `llvm::MutableArr
Endilll wrote:
I think I should post here an example from our codebase that made me think
twice about this PR.
First, there's a `MultiLevelTemplateArgumentList` (MLTAL for short), which I
believe is one of the backbones of our template engine:
https://github.com/llvm/llvm-project/blob/42b4be6
mizvekov wrote:
One suggestion, if you are going to go about this problem systematically, try
to tackle the hardest problems first.
For example: Try to make `MultiLevelTemplateArgumentList` const correct. I
would like other suggestions as well.
https://github.com/llvm/llvm-project/pull/93493
mizvekov wrote:
> That's the opposite of my view. Mutation needs to be justified. "What if we
> need it later" can be used to justify anything, and if we do need it later
> then we change the code then. Until that point, readers can see `const` and
> know that things aren't being changed out f
davidstone wrote:
> > Errr, not certain I agree with this -- that basically is "admit defeat and
> > stop aiming for const correctness."
>
> Well, I am saying, add const to places we are pretty sure we will never
> change, and leave const out when in doubt. Don't add const just because we
> d
mizvekov wrote:
> Errr, not certain I agree with this -- that basically is "admit defeat and
> stop aiming for const correctness."
Well, I am saying, add const to places we are pretty sure we will never change,
and leave const out when in doubt.
Don't add const just because we don't need mutat
@@ -817,11 +817,11 @@ class alignas(8) Decl {
"owned local decl but no local module storage");
return reinterpret_cast(this)[-1];
}
- void setLocalOwningModule(Module *M) {
+ void setLocalOwningModule(const Module *M) {
AaronBallman wrote:
T
AaronBallman wrote:
> My philosophy on problems like that is that the first step to getting out of
> a hole is to stop digging the hole. We aren't going to make all of clang
> const-correct overnight, right?
Yup! I think we're in agreement with the long-term goal and how we get there.
The de
mizvekov wrote:
I could agree on being const correct here, but mostly by removing const, not
adding more, in the general case.
The problem here is that this is such a big code base, and where some times
parameters can be passed down from function to function for a very long depth,
that this c
davidstone wrote:
My philosophy on problems like that is that the first step to getting out of a
hole is to stop digging the hole. We aren't going to make all of clang
const-correct overnight, right? My goal here is starting with this one type:
`Module`, and working on it. To me, this commit *
AaronBallman wrote:
You don't have to convince me that being const-correct is a good thing, I'm
already in full support of that. :-)
The push back isn't "no, don't do this", it's "we're not const correct in a
*lot* of places and so adding const correctness to a part of the compiler
without co
davidstone wrote:
I'll admit, I'm surprised this PR has any push-back at all since this feels
like just applying a very generally accepted principle. Things should be const
by default -- you need a reason to declare something as not const, you should
never need a reason to declare something as
https://github.com/AaronBallman edited
https://github.com/llvm/llvm-project/pull/93493
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/AaronBallman commented:
In general, I'm in favor of improving const-correctness like this. That said, I
share the concerns of other reviewers that we should be sure we're adding
const-correctness where it makes sense to do so. e.g., functions named
`getSomething()` that retu
ChuanqiXu9 wrote:
> > Can you make sure that at every place this PR touches `const` makes sense?
> > I found out recently that we can be quite good at pretending that something
> > is `const`, all the way down until we realize we need a `const_cast`,
> > because modification is required in tha
davidstone wrote:
Yes. Is there anything else you want to see in this PR before it can be merged?
(I don't have merge permissions). This is the type of PR likely to get lots of
conflicts if it stays open for long, so I'd like to get it wrapped up as fast
as possible.
https://github.com/llvm/l
Endilll wrote:
Sounds like you have a plan after this PR, which is good. I was worried that
you're just applying `const` where it doesn't cause issues _today_. Such
approach to const-correctness has been failing us (it leads to `const_cast`s
down the usage chain).
https://github.com/llvm/llvm
davidstone wrote:
> Can you make sure that at every place this PR touches `const` makes sense? I
> found out recently that we can be quite good at pretending that something is
> `const`, all the way down until we realize we need a `const_cast`, because
> modification is required in that one pl
davidstone wrote:
> Can you make sure that at every place this PR touches `const` makes sense? I
> found out recently that we can be quite good at pretending that something is
> `const`, all the way down until we realize we need a `const_cast`, because
> modification is required in that one pl
Endilll wrote:
Can you make sure that at every place this PR touches `const` makes sense? I
found out recently that we can be quite good at pretending that something is
`const`, all the way down until we realize we need a `const_cast`, because
modification is required in that one place.
https
llvmbot wrote:
@llvm/pr-subscribers-debuginfo
Author: David Stone (davidstone)
Changes
---
Patch is 129.44 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/93493.diff
59 Files Affected:
- (modified) clang/include/clang/APINotes/APINotesManag
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: David Stone (davidstone)
Changes
---
Patch is 129.44 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/93493.diff
59 Files Affected:
- (modified) clang/include/clang/APINotes/APINotesManager.h
30 matches
Mail list logo
