shuaiwang added a comment.
I've updated https://reviews.llvm.org/D45679 and I think the `isModified()`
function there is now sufficiently covering the cases you've covered here and
can be used as a good starting version if you plan to use it here.
I copied your const-values test cases and it now passes with the following
differences:
- All unused local variables removed, my check will issue a warning for them
because technically they're not modified, but I understand why you don't want
to cover them. I don't feel strongly about it and I removed it from my
copy-pasted test cases because I just to demonstrate `isModified()`
- Better recall in `some_reference_taking`, both `np_local0` and `r1_np_local0`
can be caught, similar for `np_local1` and `non_const_ref` in
`handle_from_array`.
- `direct_class_access` `np_local5` triggers with my check (shouldn't it?)
- In `range_for` non-const loop variables triggers with my check (shouldn't
they?)
- In `range_for` local arrays doesn't trigger with my check, because I'm
treating ArrayToPointerDecay the same as taking address of a variable, and
looping over an array would involve ArrayToPointerDecay when the implicit
`__begin`/`__end` are initialized. I added a note inside `isModified` for
future improvements.
- My check over triggers for template (with my failed attempt to fix), but I
think it's more of some mistake in the check itself rather than in `isModified`
>> Also when marking decls as "can't be const" we'll have to do record
>> separately for modifying behaviors coming from different functions.
>> Which of course are all possible but code will get too complex than
>> necessary IMO.
>
> I cant follow you on that one. I consider every path that allows future
> modification (like taking a non-const reference/pointer) as `cant be const`.
> That would be enough, wouldnt it?
> A separate function can only modify a variable if it has some form of
> non-const handle to it, which must have been created at some point.
Sorry about the confusion.
Basically consider this example:
class Foo {
public:
void a() { x = 10; }
void b() { // nothing }
void c() { a(); }
void d() { b(); }
private:
int x;
};
If we check whether `isModified(dereference(cxxThisExpr()))` for each
`CompondStmt(hasParent(functionDecl()))`, we would conclude that:
- `a()` should stay non-const, because there's `this->x = 10` modifying `*this`
- `b()` should be changed to const, because nothing modifies `*this`
- `c()` should stay non-const, because we're invoking non-const member function
on `this`
- `d()` should also stay non-const, with the same reason for c(). (We can in
theory be smarter about this one because the definition of b() happens to be
inside the same TU but that's out of scope right now)
However if we use a per-TU map to record whether `x` can be const, we'll
conclude that `x` is modified thus can't be const, missing the one that `b()`
is not modifying `x`. To fix that we'll need two-layered map recording that `x`
is only modified in `a()` and potentially modified indirectly from `c()` and
`d()`, so that in the end we can figure out that `b()` is safe to be marked
const.
Anyway, all I was trying to say is: let's use the `isModified()` approach as
it's simpler & cleaner for future use cases like adding const to member
functions. And it feels to me that we've already reached agreement there :)
> @shuaiwang Are you in IRC from time to time? I think it will be better to
> discuss changes in chat, when i start to introduce your approach here.
Not really, but I can get myself familiar with it.
>
>
>> isModified(Expr, Stmt), that checks whether Expr is (potentially) modified
>> within Stmt, is something I believe to be quite useful:
>
> What i understand from your comments and code:
>
> - implement a utility taking `DeclRefExpr` and check if there is modification
> in some scope (`Stmt`, e.g. `CompoundStmt`) -> `isModified` or
> `cantBeModifiedWithin`
It doesn't have to be `DeclRefExpr`, any `Expr` should work, and it's useful to
make it accept any `Expr`:
- It can be used to check whether `dereference(cxxThisExpr())` is modified or
not.
- For pointer types, it can be used to check both
`declRefExpr(isPointerType())` and `dereference(declRefExpr(isPointerType()))`,
and suggest adding const at different level
> - match all relevant non-const variable declarations as potential candidates
> - track all uses of the candidates and check for modification in their scope
>
> One Note: We could include variables in unnamed namespaces and `static`
> declared variables. They have TU scope.
Great catch!
> My deviations:
>
> - a variable should be considered modified if a non-const handle is create
> from it, even if that handle does not modify its referenced value. As first
> step, those handles should be marked const, then the original value can be
> marked as const. That is required to produce compiling code after potential
> code-transformation.
I believe we can issue warning + fixit within one pass:
int f() {
int a = 10;
int& b = a;
return b;
}
We should be able to issue warnings for both `a` and `b`, because `b` itself is
a `varDecl` without modifying behavior, and when checking `a` we'll just repeat
a bit more work that was already done when checking `b` by following the
reference chain to be able to find that `a` can be const as well.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45444
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
