leonardchan marked 2 inline comments as done.
leonardchan added inline comments.
Comment at: lib/Sema/SemaType.cpp:4913-4916
+ExpectNoDerefChunk = false;
+ }
+
+ ExpectNoDerefChunk = state.didParseNoDeref();
dblaikie wrote:
> Pointed out in PR41
dblaikie added inline comments.
Comment at: lib/Sema/SemaType.cpp:4913-4916
+ExpectNoDerefChunk = false;
+ }
+
+ ExpectNoDerefChunk = state.didParseNoDeref();
Pointed out in PR41774 there's a dead store to ExpectNoDerefChunk here.
Should line 4
This revision was automatically updated to reflect the committed changes.
Closed by commit rC348442: [Sema/Attribute] Check for noderef attribute
(authored by leonardchan, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D49511?vs=176907&id=176908#toc
Repository:
rC Clang
C
leonardchan updated this revision to Diff 176907.
leonardchan marked an inline comment as done.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D49511/new/
https://reviews.llvm.org/D49511
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs
rsmith accepted this revision.
rsmith added a comment.
Thanks for your patience.
Comment at: clang/lib/Sema/SemaInit.cpp:7838-7854
+ if (const auto *FromPtrType = SourceType->getAs()) {
+if (const auto *ToPtrType = Step->Type->getAs()) {
+ if (FromPtrType-
leonardchan added a comment.
*ping* @rsmith Any more comments on this patch or the one before it
(https://reviews.llvm.org/D54014)? That one has the fix for pushing and popping
another ExprEvalContext before acting on a function body in this patch.
Repository:
rC Clang
CHANGES SINCE LAST AC
leonardchan updated this revision to Diff 172695.
leonardchan marked 5 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticGroups.td
clang/includ
leonardchan added inline comments.
Comment at: lib/Parse/ParseStmt.cpp:102-104
+ Actions.PushExpressionEvaluationContext(
+ Actions.ExprEvalContexts.back().Context);
ParenBraceBracketBalancer BalancerRAIIObj(*this);
leonardchan wrote:
> rsmith wrote:
> >
leonardchan marked 2 inline comments as done.
leonardchan added inline comments.
Comment at: lib/Parse/ParseStmt.cpp:102-104
+ Actions.PushExpressionEvaluationContext(
+ Actions.ExprEvalContexts.back().Context);
ParenBraceBracketBalancer BalancerRAIIObj(*this);
-
mcgrathr added a comment.
The motivating use case always pairs `noderef` with an `address_space`
attribute, and it's already invalid to convert between pointer types in
different address spaces without a cast.
So I don't think we have a strong opinion one way or the other. In the
abstract, I'd
rsmith added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:3513
+ int x = 2;
+ int &y = x; // warning: 'noderef' can only be used on an array or pointer
type
+
Should `noderef` appear somewhere in this example? :)
Comment at:
leonardchan updated this revision to Diff 164706.
leonardchan added a comment.
- Push and pop contexts for every parsed statement inside
`ParseStatementOrDeclaration` instead of at the start and end of compound
statements
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
includ
leonardchan updated this revision to Diff 164136.
leonardchan added a comment.
- Push and pop a new ExpressionEvaluationContext when we enter and exit a
compound statement.
- Remove Start/StopCheckingNoderef functions since we can now warn whenever we
pop
Repository:
rC Clang
https://review
leonardchan added a comment.
@rsmith ping
Repository:
rC Clang
https://reviews.llvm.org/D49511
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
leonardchan added inline comments.
Comment at: lib/Parse/ParseExpr.cpp:1126
+
+Actions.StartCheckingNoDeref();
+
rsmith wrote:
> leonardchan wrote:
> > rsmith wrote:
> > > This parser-driven start/stop mechanism will not work in C++ templates.
> > > Instead,
leonardchan updated this revision to Diff 162126.
leonardchan marked 13 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/Diagnosti
rsmith added inline comments.
Comment at: lib/Parse/ParseExpr.cpp:1126
+
+Actions.StartCheckingNoDeref();
+
leonardchan wrote:
> rsmith wrote:
> > This parser-driven start/stop mechanism will not work in C++ templates.
> > Instead, can you remove the "start"
leonardchan added a comment.
In https://reviews.llvm.org/D49511#1206267, @rsmith wrote:
> In https://reviews.llvm.org/D49511#1206265, @rsmith wrote:
>
> > In https://reviews.llvm.org/D49511#1194716, @leonardchan wrote:
> >
> > > @rsmith any more feedback on this current version? If it still looks
leonardchan added inline comments.
Comment at: lib/Parse/ParseExpr.cpp:1126
+
+Actions.StartCheckingNoDeref();
+
rsmith wrote:
> This parser-driven start/stop mechanism will not work in C++ templates.
> Instead, can you remove the "start" part and check the
leonardchan updated this revision to Diff 161785.
leonardchan marked 6 inline comments as done and an inline comment as not done.
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticGroups.t
rsmith added a comment.
In https://reviews.llvm.org/D49511#1206265, @rsmith wrote:
> In https://reviews.llvm.org/D49511#1194716, @leonardchan wrote:
>
> > @rsmith any more feedback on this current version? If it still looks
> > incorrect to use the record this way, I don't mind simplifying it to
rsmith added a comment.
In https://reviews.llvm.org/D49511#1194716, @leonardchan wrote:
> @rsmith any more feedback on this current version? If it still looks
> incorrect to use the record this way, I don't mind simplifying it to work on
> lvalue to rvalue conversions without checking for a lea
rsmith added inline comments.
Comment at: lib/Parse/ParseExpr.cpp:1126
+
+Actions.StartCheckingNoDeref();
+
This parser-driven start/stop mechanism will not work in C++ templates.
Instead, can you remove the "start" part and check the noderef exprs as part o
leonardchan updated this revision to Diff 160892.
leonardchan marked 6 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticGroups.td
inc
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
A few more minor nits to be cleared up, but otherwise LGTM. You should wait for
@rsmith to sign off before committing in case he has further comments, however.
leonardchan added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:14249
+
+if (Sema::TypeHasNoDeref(Inner))
+ DeclRef = E;
aaron.ballman wrote:
> leonardchan wrote:
> > aaron.ballman wrote:
> > > The sugar was stripped off at the pointer level, but no
leonardchan updated this revision to Diff 160451.
leonardchan marked 3 inline comments as done.
leonardchan added a comment.
- Remove sugar from pointee types
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:14249
+
+if (Sema::TypeHasNoDeref(Inner))
+ DeclRef = E;
leonardchan wrote:
> aaron.ballman wrote:
> > The sugar was stripped off at the pointer level, but not at the pointee
> > lev
leonardchan added a comment.
@rsmith
Comment at: lib/Sema/SemaExpr.cpp:14249
+
+if (Sema::TypeHasNoDeref(Inner))
+ DeclRef = E;
aaron.ballman wrote:
> The sugar was stripped off at the pointer level, but not at the pointee
> level. e.g.,
> ```
> typed
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:14249
+
+if (Sema::TypeHasNoDeref(Inner))
+ DeclRef = E;
The sugar was stripped off at the pointer level, but not at the pointee level.
e.g.,
```
typedef int (bobble);
typedef bobble
leonardchan updated this revision to Diff 160403.
leonardchan marked an inline comment as done.
leonardchan added a comment.
- Checks for sugared types and expressions wrapped in parenthesis
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/AST/Type.h
include/clan
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:14251-14253
+if (const auto *Ptr = dyn_cast(Ty))
+ Inner = Ptr->getPointeeType();
+else if (const auto *Arr = dyn_cast(Ty))
I don't think this strips off sugar from the type, so I
aaron.ballman added a comment.
Oops, I hit "Send" too soon. I was going to say that you should also keep an
eye on https://reviews.llvm.org/D50526 because that may impact this patch (or
vice versa if this one lands first).
Repository:
rC Clang
https://reviews.llvm.org/D49511
leonardchan added a comment.
@rsmith any more feedback on this current version? If it still looks incorrect
to use the record this way, I don't mind simplifying it to work on lvalue to
rvalue conversions without checking for a leading address space operation.
Repository:
rC Clang
https://re
leonardchan updated this revision to Diff 159409.
leonardchan marked 3 inline comments as done.
leonardchan added a comment.
- Changed tick to single quote in diagnostic
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/AST/Type.h
include/clang/Basic/Attr.td
inc
aaron.ballman added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:3489
+
+``noderef`` is currently only supported for C style pointers and arrays and
not usable for
+references or Objective-C pointers.
I would drop the "C style" and just say it's
leonardchan updated this revision to Diff 158423.
leonardchan added a comment.
- Moved the counter and set into `ExpressionEvaluationContextRecord`
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrD
leonardchan added a comment.
> Your current counter-based approach doesn't work very well in the case where
> we switch to another context while processing an expression (for example,
> during template instantiation): you'll defer all the diagnostics for the
> inner construct until the outer co
leonardchan added a comment.
> You shouldn't be adding your own `ExpressionEvaluationContextRecord`s. What I
> was suggesting is that you store a list of pending `noderef` expressions on
> the record, and diagnose them when the record is popped (if they've not been
> removed since).
This is
leonardchan updated this revision to Diff 158407.
leonardchan marked 6 inline comments as done.
leonardchan added a comment.
- Added tests for checking that `noderef` can only be used for C style pointers
and arrays (not usable for references or ObjC pointers)
- Added warnings for dereferencing a
mcgrathr added a comment.
I think this wants to be a hard error rather than a warning. Though since we
use -Werror anyway if others feel strongly contrary I won't object.
Repository:
rC Clang
https://reviews.llvm.org/D49511
___
cfe-commits mail
rsmith added a comment.
In https://reviews.llvm.org/D49511#1170693, @leonardchan wrote:
> Done. I opted for not using `ExpressionEvaluationContextRecord` because I
> don't think I was using it correctly. A few other tests in sema failed when I
> tried pushing and popping from the stack holding
aaron.ballman added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:3426
+ let Content = [{
+The ``noderef`` attribute causes clang to throw a warning whenever a pointer
marked with
+this attribute is dereferenced. This is ideally used with pointers that point
to
leonardchan added a comment.
@aaron.ballman @rsmith Any more feedback on this patch?
Repository:
rC Clang
https://reviews.llvm.org/D49511
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
leonardchan added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:3426
+ let Content = [{
+The ``noderef`` attribute causes clang to throw a warning whenever a pointer
marked with
+this attribute is dereferenced. This is ideally used with pointers that point
to sp
leonardchan updated this revision to Diff 156826.
leonardchan marked 21 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
aaron.ballman added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:3426
+ let Content = [{
+The ``noderef`` attribute causes clang to throw a warning whenever a pointer
marked with
+this attribute is dereferenced. This is ideally used with pointers that point
to
leonardchan added a comment.
In https://reviews.llvm.org/D49511#1168692, @rsmith wrote:
> The way in which you're checking for the problematic cases is unnecessarily
> expensive. Instead of performing a separate AST traversal, please detect
> whether you should be producing the warning directly
leonardchan updated this revision to Diff 156634.
leonardchan marked 2 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/AST/Type.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticGroups.td
inc
erik.pilkington added a reviewer: aaron.ballman.
erik.pilkington added a comment.
Thanks for working on this! CCing Aaron, who is code owner for attributes.
Comment at: include/clang/Basic/AttrDocs.td:3355
+ let Content = [{
+The ``noderef`` attribute allows for showing a warn
rsmith added a comment.
The way in which you're checking for the problematic cases is unnecessarily
expensive. Instead of performing a separate AST traversal, please detect
whether you should be producing the warning directly when forming the
problematic expressions. (For example, you could sto
leonardchan updated this revision to Diff 156167.
leonardchan added a comment.
- Added checks for expressions in statements other than declarations or
expression statements
Repository:
rC Clang
https://reviews.llvm.org/D49511
Files:
include/clang/AST/Type.h
include/clang/Basic/Attr.td
leonardchan created this revision.
leonardchan added reviewers: phosek, mcgrathr.
leonardchan added a project: clang.
This patch adds the `noderef` attribute in clang and checks for dereferences of
types that have this attribute. This attribute is currently used by sparse and
would like to be po
53 matches
Mail list logo
