[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added a comment. In https://reviews.llvm.org/D43248#1036439, @jdenny wrote: > So, I'm planning to remove test/Frontend/ast-attr.cpp, rename > test/Sema/attr-print.cpp to test/Misc/attr-print-emit.cpp, and change its run > lines to: > > // RUN: %clang_cc1 %s -ast-print | FileCheck %s > // RUN: %clang -emit-ast -o %t.ast %s > // RUN: %clang_cc1 %t.ast -ast-print | FileCheck %s > > > That seems to work fine locally. I'll just leave the old > test/Sema/attr-print.c alone as it's not part of this problem. Any > objections? I went ahead and committed. It's in r327456. Repository: rC Clang https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added a comment. In https://reviews.llvm.org/D43248#1036427, @echristo wrote: > In https://reviews.llvm.org/D43248#1036426, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D43248#1036409, @jdenny wrote: > > > > > I'd prefer to move it than to expect people to obey such a comment. > > > Let's see what Aaron says. > > > > > > I have a slight preference for moving the tests now that I know they're > > causing problems, unless that turns out to be overly onerous for some > > reason. > > > > Thank you, @echristo for pointing out the issues with the tests, I hadn't > > considered that. > > > No worries :) > > That said, please don't revert and reapply to move. Just a followup commit is > just fine - and whenever you get a chance. (Though if you let me know when > you do I'd appreciate it :) Sure. So, I'm planning to remove test/Frontend/ast-attr.cpp, rename test/Sema/attr-print.cpp to test/Misc/attr-print-emit.cpp, and change its run lines to: // RUN: %clang_cc1 %s -ast-print | FileCheck %s // RUN: %clang -emit-ast -o %t.ast %s // RUN: %clang_cc1 %t.ast -ast-print | FileCheck %s That seems to work fine locally. I'll just leave the old test/Sema/attr-print.c alone as it's not part of this problem. Any objections? Repository: rC Clang https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
echristo added a comment. In https://reviews.llvm.org/D43248#1036426, @aaron.ballman wrote: > In https://reviews.llvm.org/D43248#1036409, @jdenny wrote: > > > I'd prefer to move it than to expect people to obey such a comment. Let's > > see what Aaron says. > > > I have a slight preference for moving the tests now that I know they're > causing problems, unless that turns out to be overly onerous for some reason. > > Thank you, @echristo for pointing out the issues with the tests, I hadn't > considered that. No worries :) That said, please don't revert and reapply to move. Just a followup commit is just fine - and whenever you get a chance. (Though if you let me know when you do I'd appreciate it :) Repository: rC Clang https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added a comment. In https://reviews.llvm.org/D43248#1036409, @jdenny wrote: > I'd prefer to move it than to expect people to obey such a comment. Let's > see what Aaron says. I have a slight preference for moving the tests now that I know they're causing problems, unless that turns out to be overly onerous for some reason. Thank you, @echristo for pointing out the issues with the tests, I hadn't considered that. Repository: rC Clang https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added a comment. I'd prefer to move it than to expect people to obey such a comment. Let's see what Aaron says. Repository: rC Clang https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
echristo added inline comments. Comment at: test/Sema/attr-print.cpp:3 +// This file is also used as input for %S/../Frontend/ast-attr.cpp. + jdenny wrote: > echristo wrote: > > Relatedly I don't think we use files as input files to other directories > > and I don't think we should really. What are you trying to test here? This > > breaks the hermeticness of each particular test directory. > Grep for "%S/.." and you'll see that a few other tests do something like this > as well. > > In test/Sema/attr-print.cpp, I am testing printing of attributes. I chose to > put that there because of the existing attr-print.c there. > > In test/Frontend/ast-attr.cpp, I am testing serialization of attributes. I > chose to put that there because I see other -emit-ast tests there and > because, if I put it in test/Sema instead, I get the complaint "Do not use > the driver in Sema tests". > > The same C++ code makes sense in both of these, but replicating it in two > files would worsen maintainability. > > I could try to combine into one file in, say, tests/Misc if that works. > > I have no strong opinions on where these live. Just for my own education, is > something real breaking right now because of their current locations? > > Thanks. Basically it's breaking an external build system (bazel) that has fairly distinct and specific dependency requirements and so layering and other dependencies are pretty entertaining. Right now we avoid testing those particular tests and have TODOs of fixing them and the requirements to use a single directory and I was trying to avoid one more here. All of that said I totally understand the desire to keep the maintenance burden minimized and an external build system shouldn't affect whether or not we do one particular thing or another - I was trying to get it written so that we could enable it without much undue burden. I'm uncertain if a comment of: // If you change this file you should also change blah file. or moving it to another directory where you can do both tests at the same time. Repository: rC Clang https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added inline comments. Comment at: test/Sema/attr-print.cpp:3 +// This file is also used as input for %S/../Frontend/ast-attr.cpp. + echristo wrote: > Relatedly I don't think we use files as input files to other directories and > I don't think we should really. What are you trying to test here? This breaks > the hermeticness of each particular test directory. Grep for "%S/.." and you'll see that a few other tests do something like this as well. In test/Sema/attr-print.cpp, I am testing printing of attributes. I chose to put that there because of the existing attr-print.c there. In test/Frontend/ast-attr.cpp, I am testing serialization of attributes. I chose to put that there because I see other -emit-ast tests there and because, if I put it in test/Sema instead, I get the complaint "Do not use the driver in Sema tests". The same C++ code makes sense in both of these, but replicating it in two files would worsen maintainability. I could try to combine into one file in, say, tests/Misc if that works. I have no strong opinions on where these live. Just for my own education, is something real breaking right now because of their current locations? Thanks. Repository: rC Clang https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
echristo added inline comments. Comment at: test/Sema/attr-print.cpp:3 +// This file is also used as input for %S/../Frontend/ast-attr.cpp. + Relatedly I don't think we use files as input files to other directories and I don't think we should really. What are you trying to test here? This breaks the hermeticness of each particular test directory. Repository: rC Clang https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
This revision was automatically updated to reflect the committed changes.
jdenny marked 8 inline comments as done.
Closed by commit rC327405: Reland [Attr] Fix parameter indexing for
several attributes (authored by jdenny, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D43248?vs=138113=138193#toc
Repository:
rC Clang
https://reviews.llvm.org/D43248
Files:
include/clang/AST/Attr.h
include/clang/Basic/Attr.td
lib/AST/ExprConstant.cpp
lib/CodeGen/CGCall.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
test/CodeGenCXX/alloc-size.cpp
test/Frontend/ast-attr.cpp
test/Misc/ast-dump-attr.cpp
test/Sema/attr-ownership.cpp
test/Sema/attr-print.cpp
test/Sema/error-type-safety.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1231,9 +1231,10 @@
if (Att->getModule() != II_malloc)
return nullptr;
- OwnershipAttr::args_iterator I = Att->args_begin(), E = Att->args_end();
+ ParamIdx *I = Att->args_begin(), *E = Att->args_end();
if (I != E) {
-return MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(), State);
+return MallocMemAux(C, CE, CE->getArg(I->getASTIndex()), UndefinedVal(),
+State);
}
return MallocMemAux(C, CE, UnknownVal(), UndefinedVal(), State);
}
@@ -1331,9 +1332,9 @@
bool ReleasedAllocated = false;
for (const auto : Att->args()) {
-ProgramStateRef StateI = FreeMemAux(C, CE, State, Arg,
- Att->getOwnKind() == OwnershipAttr::Holds,
- ReleasedAllocated);
+ProgramStateRef StateI = FreeMemAux(
+C, CE, State, Arg.getASTIndex(),
+Att->getOwnKind() == OwnershipAttr::Holds, ReleasedAllocated);
if (StateI)
State = StateI;
}
Index: lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -54,10 +54,11 @@
AttrNonNull.set(0, NumArgs);
break;
}
-for (unsigned Val : NonNull->args()) {
- if (Val >= NumArgs)
+for (const ParamIdx : NonNull->args()) {
+ unsigned IdxAST = Idx.getASTIndex();
+ if (IdxAST >= NumArgs)
continue;
- AttrNonNull.set(Val);
+ AttrNonNull.set(IdxAST);
}
}
return AttrNonNull;
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -5475,9 +5475,8 @@
llvm::APInt ) {
const AllocSizeAttr *AllocSize = getAllocSizeAttr(Call);
- // alloc_size args are 1-indexed, 0 means not present.
- assert(AllocSize && AllocSize->getElemSizeParam() != 0);
- unsigned SizeArgNo = AllocSize->getElemSizeParam() - 1;
+ assert(AllocSize && AllocSize->getElemSizeParam().isValid());
+ unsigned SizeArgNo = AllocSize->getElemSizeParam().getASTIndex();
unsigned BitsInSizeT = Ctx.getTypeSize(Ctx.getSizeType());
if (Call->getNumArgs() <= SizeArgNo)
return false;
@@ -5495,14 +5494,13 @@
if (!EvaluateAsSizeT(Call->getArg(SizeArgNo), SizeOfElem))
return false;
- if (!AllocSize->getNumElemsParam()) {
+ if (!AllocSize->getNumElemsParam().isValid()) {
Result = std::move(SizeOfElem);
return true;
}
APSInt NumberOfElems;
- // Argument numbers start at 1
- unsigned NumArgNo = AllocSize->getNumElemsParam() - 1;
+ unsigned NumArgNo = AllocSize->getNumElemsParam().getASTIndex();
if (!EvaluateAsSizeT(Call->getArg(NumArgNo), NumberOfElems))
return false;
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -176,7 +176,8 @@
Sema , const MultiLevelTemplateArgumentList ,
const AllocAlignAttr *Align, Decl *New) {
Expr *Param = IntegerLiteral::Create(
- S.getASTContext(), llvm::APInt(64, Align->getParamIndex()),
+ S.getASTContext(),
+ llvm::APInt(64, Align->getParamIndex().getSourceIndex()),
S.getASTContext().UnsignedLongLongTy, Align->getLocation());
S.AddAllocAlignAttr(Align->getLocation(), New, Param,
Align->getSpellingListIndex());
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -13185,7 +13185,7 @@
// We already have a __builtin___CFStringMakeConstantString,
// but builds that
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added a comment. In https://reviews.llvm.org/D43248#1035509, @aaron.ballman wrote: > LGTM with the test comments fixed up. Thanks! I'll commit tomorrow. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with the test comments fixed up. Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:5 +// RUN: %clang -emit-ast -o %t.ast %S/../Sema/attr-print.cpp +// RUN: %clang_cc1 %t.ast -ast-print | FileCheck %S/../Sema/attr-print.cpp jdenny wrote: > aaron.ballman wrote: > > Just to verify my understanding, this test is checking the serialization > > and deserialization? > That's right, and it failed without the other changes in this revision > because alloc_size's second argument serialized/deserialized as 0 when it was > actually invalid (that is, unspecified). Of course, this test is more > thorough than just exercising alloc_size. Perfect, thank you! https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added inline comments. Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:5 +// RUN: %clang -emit-ast -o %t.ast %S/../Sema/attr-print.cpp +// RUN: %clang_cc1 %t.ast -ast-print | FileCheck %S/../Sema/attr-print.cpp aaron.ballman wrote: > Just to verify my understanding, this test is checking the serialization and > deserialization? That's right, and it failed without the other changes in this revision because alloc_size's second argument serialized/deserialized as 0 when it was actually invalid (that is, unspecified). Of course, this test is more thorough than just exercising alloc_size. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added inline comments. Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:5 +// RUN: %clang -emit-ast -o %t.ast %S/../Sema/attr-print.cpp +// RUN: %clang_cc1 %t.ast -ast-print | FileCheck %S/../Sema/attr-print.cpp Just to verify my understanding, this test is checking the serialization and deserialization? https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added inline comments. Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:1-2 +// %S/../Sema/attr-print.cpp exercises many different attributes, so we reuse +// it here to check -emit-ast for attributes. + aaron.ballman wrote: > jdenny wrote: > > aaron.ballman wrote: > > > Can you move this below the RUN line? > > Sure. I'm still trying to learn the LLVM coding standards. Is this > > specified there? > Nope! I'm just used to looking at the very first line of the test to know > what it's running, and that seems consistent with other tests. OK, that makes sense. I'll be sure to change both of those. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added a comment. In https://reviews.llvm.org/D43248#1035477, @jdenny wrote: > In https://reviews.llvm.org/D43248#1035466, @aaron.ballman wrote: > > > It seems like there are some other changes than just the serialize and > > deserialize that I'm not opposed to, but am wondering why they're needed. > > It seems some functions are now `getFoo()` calls > > > These were originally named getFoo, and my previous patch changed them to > foo. I believe I did that to make ParamIdxArgument accessors more like > VariadicParamIdxArgument accessors (which inherits accessors from > VariadicArgument), but I probably shouldn't have done that. In any case, > this new revision implements ParamIdxArgument using SimpleArgument, and that > names accessors like getFoo. Ahhh, thank you for the explanation, that makes sense. >> and it seems like some declarations moved around. Are those intended as part >> of this patch? > > Are you referring to the changes in SemaDeclAttr.cpp? Those changes are > needed because the ParamIdx constructor now asserts that Idx is one-origin, > but that requires validating that it's actually one-origin beforehand. > Sorry, I should've mentioned the new asserts. Ah, okay, thank you! Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:1-2 +// %S/../Sema/attr-print.cpp exercises many different attributes, so we reuse +// it here to check -emit-ast for attributes. + jdenny wrote: > aaron.ballman wrote: > > Can you move this below the RUN line? > Sure. I'm still trying to learn the LLVM coding standards. Is this > specified there? Nope! I'm just used to looking at the very first line of the test to know what it's running, and that seems consistent with other tests. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added inline comments. Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:1-2 +// %S/../Sema/attr-print.cpp exercises many different attributes, so we reuse +// it here to check -emit-ast for attributes. + aaron.ballman wrote: > Can you move this below the RUN line? Sure. I'm still trying to learn the LLVM coding standards. Is this specified there? https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added a comment. In https://reviews.llvm.org/D43248#1035466, @aaron.ballman wrote: > It seems like there are some other changes than just the serialize and > deserialize that I'm not opposed to, but am wondering why they're needed. It > seems some functions are now `getFoo()` calls These were originally named getFoo, and my previous patch changed them to foo. I believe I did that to make ParamIdxArgument accessors more like VariadicParamIdxArgument accessors (which inherits accessors from VariadicArgument), but I probably shouldn't have done that. In any case, this new revision implements ParamIdxArgument using SimpleArgument, and that names accessors like getFoo. > and it seems like some declarations moved around. Are those intended as part > of this patch? Are you referring to the changes in SemaDeclAttr.cpp? Those changes are needed because the ParamIdx constructor now asserts that Idx is one-origin, but that requires validating that it's actually one-origin beforehand. Sorry, I should've mentioned the new asserts. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added a comment. It seems like there are some other changes than just the serialize and deserialize that I'm not opposed to, but am wondering why they're needed. It seems some functions are now `getFoo()` calls and it seems like some declarations moved around. Are those intended as part of this patch? Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:1-2 +// %S/../Sema/attr-print.cpp exercises many different attributes, so we reuse +// it here to check -emit-ast for attributes. + Can you move this below the RUN line? Comment at: cfe/trunk/test/Sema/attr-print.cpp:1 +// This file is also used as input for %S/../Frontend/ast-attr.cpp. + Can you move this comment below the RUN line? https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny updated this revision to Diff 138113.
jdenny added a comment.
Well, that didn't work. Here's another attempt at getting the paths right.
https://reviews.llvm.org/D43248
Files:
cfe/trunk/include/clang/AST/Attr.h
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
cfe/trunk/test/CodeGenCXX/alloc-size.cpp
cfe/trunk/test/Frontend/ast-attr.cpp
cfe/trunk/test/Misc/ast-dump-attr.cpp
cfe/trunk/test/Sema/attr-ownership.cpp
cfe/trunk/test/Sema/attr-print.cpp
cfe/trunk/test/Sema/error-type-safety.cpp
cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
Index: cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
===
--- cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
+++ cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
@@ -104,6 +104,7 @@
.Case("Expr *", "Record.readExpr()")
.Case("IdentifierInfo *", "Record.getIdentifierInfo()")
.Case("StringRef", "Record.readString()")
+.Case("ParamIdx", "ParamIdx::deserialize(Record.readInt())")
.Default("Record.readInt()");
}
@@ -122,6 +123,7 @@
.Case("Expr *", "AddStmt(" + std::string(name) + ");\n")
.Case("IdentifierInfo *", "AddIdentifierRef(" + std::string(name) + ");\n")
.Case("StringRef", "AddString(" + std::string(name) + ");\n")
+.Case("ParamIdx", "push_back(" + std::string(name) + ".serialize());\n")
.Default("push_back(" + std::string(name) + ");\n");
}
@@ -302,9 +304,8 @@
std::string getIsOmitted() const override {
if (type == "IdentifierInfo *")
return "!get" + getUpperName().str() + "()";
- // FIXME: Do this declaratively in Attr.td.
- if (getAttrName() == "AllocSize")
-return "0 == get" + getUpperName().str() + "()";
+ if (type == "ParamIdx")
+return "!get" + getUpperName().str() + "().isValid()";
return "false";
}
@@ -319,6 +320,8 @@
<< "()->getName() : \"\") << \"";
else if (type == "TypeSourceInfo *")
OS << "\" << get" << getUpperName() << "().getAsString() << \"";
+ else if (type == "ParamIdx")
+OS << "\" << get" << getUpperName() << "().getSourceIndex() << \"";
else
OS << "\" << get" << getUpperName() << "() << \"";
}
@@ -341,6 +344,11 @@
<< getUpperName() << "\";\n";
} else if (type == "int" || type == "unsigned") {
OS << "OS << \" \" << SA->get" << getUpperName() << "();\n";
+ } else if (type == "ParamIdx") {
+if (isOptional())
+ OS << "if (SA->get" << getUpperName() << "().isValid())\n ";
+OS << "OS << \" \" << SA->get" << getUpperName()
+ << "().getSourceIndex();\n";
} else {
llvm_unreachable("Unknown SimpleArgument type!");
}
@@ -618,6 +626,10 @@
virtual void writeValueImpl(raw_ostream ) const {
OS << "OS << Val;\n";
}
+// Assumed to receive a parameter: raw_ostream OS.
+virtual void writeDumpImpl(raw_ostream ) const {
+ OS << " OS << \" \" << Val;\n";
+}
public:
VariadicArgument(const Record , StringRef Attr, std::string T)
@@ -744,7 +756,22 @@
void writeDump(raw_ostream ) const override {
OS << "for (const auto : SA->" << RangeName << "())\n";
- OS << " OS << \" \" << Val;\n";
+ writeDumpImpl(OS);
+}
+ };
+
+ class VariadicParamIdxArgument : public VariadicArgument {
+ public:
+VariadicParamIdxArgument(const Record , StringRef Attr)
+: VariadicArgument(Arg, Attr, "ParamIdx") {}
+
+ public:
+void writeValueImpl(raw_ostream ) const override {
+ OS << "OS << Val.getSourceIndex();\n";
+}
+
+void writeDumpImpl(raw_ostream ) const override {
+ OS << " OS << \" \" << Val.getSourceIndex();\n";
}
};
@@ -1247,6 +1274,10 @@
Ptr = llvm::make_unique(Arg, Attr);
else if (ArgName == "VariadicExprArgument")
Ptr = llvm::make_unique(Arg, Attr);
+ else if (ArgName == "VariadicParamIdxArgument")
+Ptr = llvm::make_unique(Arg, Attr);
+ else if (ArgName == "ParamIdxArgument")
+Ptr = llvm::make_unique(Arg, Attr, "ParamIdx");
else if (ArgName == "VersionArgument")
Ptr = llvm::make_unique(Arg, Attr);
Index: cfe/trunk/test/Sema/error-type-safety.cpp
===
--- cfe/trunk/test/Sema/error-type-safety.cpp
+++ cfe/trunk/test/Sema/error-type-safety.cpp
@@ -3,21 +3,50 @@
#define INT_TAG 42
static const int test_in
- __attribute__((type_tag_for_datatype(test, int))) = INT_TAG;
+__attribute__((type_tag_for_datatype(test, int))) = INT_TAG;
//
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny updated this revision to Diff 138111.
jdenny added a comment.
OK, this diff has the svn paths, and I've rebased to a more recent master.
https://reviews.llvm.org/D43248
Files:
trunk/include/clang/AST/Attr.h
trunk/include/clang/Basic/Attr.td
trunk/lib/AST/ExprConstant.cpp
trunk/lib/CodeGen/CGCall.cpp
trunk/lib/Sema/SemaChecking.cpp
trunk/lib/Sema/SemaDecl.cpp
trunk/lib/Sema/SemaDeclAttr.cpp
trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
trunk/test/CodeGenCXX/alloc-size.cpp
trunk/test/Frontend/ast-attr.cpp
trunk/test/Misc/ast-dump-attr.cpp
trunk/test/Sema/attr-ownership.cpp
trunk/test/Sema/attr-print.cpp
trunk/test/Sema/error-type-safety.cpp
trunk/utils/TableGen/ClangAttrEmitter.cpp
Index: trunk/utils/TableGen/ClangAttrEmitter.cpp
===
--- trunk/utils/TableGen/ClangAttrEmitter.cpp
+++ trunk/utils/TableGen/ClangAttrEmitter.cpp
@@ -104,6 +104,7 @@
.Case("Expr *", "Record.readExpr()")
.Case("IdentifierInfo *", "Record.getIdentifierInfo()")
.Case("StringRef", "Record.readString()")
+.Case("ParamIdx", "ParamIdx::deserialize(Record.readInt())")
.Default("Record.readInt()");
}
@@ -122,6 +123,7 @@
.Case("Expr *", "AddStmt(" + std::string(name) + ");\n")
.Case("IdentifierInfo *", "AddIdentifierRef(" + std::string(name) + ");\n")
.Case("StringRef", "AddString(" + std::string(name) + ");\n")
+.Case("ParamIdx", "push_back(" + std::string(name) + ".serialize());\n")
.Default("push_back(" + std::string(name) + ");\n");
}
@@ -302,9 +304,8 @@
std::string getIsOmitted() const override {
if (type == "IdentifierInfo *")
return "!get" + getUpperName().str() + "()";
- // FIXME: Do this declaratively in Attr.td.
- if (getAttrName() == "AllocSize")
-return "0 == get" + getUpperName().str() + "()";
+ if (type == "ParamIdx")
+return "!get" + getUpperName().str() + "().isValid()";
return "false";
}
@@ -319,6 +320,8 @@
<< "()->getName() : \"\") << \"";
else if (type == "TypeSourceInfo *")
OS << "\" << get" << getUpperName() << "().getAsString() << \"";
+ else if (type == "ParamIdx")
+OS << "\" << get" << getUpperName() << "().getSourceIndex() << \"";
else
OS << "\" << get" << getUpperName() << "() << \"";
}
@@ -341,6 +344,11 @@
<< getUpperName() << "\";\n";
} else if (type == "int" || type == "unsigned") {
OS << "OS << \" \" << SA->get" << getUpperName() << "();\n";
+ } else if (type == "ParamIdx") {
+if (isOptional())
+ OS << "if (SA->get" << getUpperName() << "().isValid())\n ";
+OS << "OS << \" \" << SA->get" << getUpperName()
+ << "().getSourceIndex();\n";
} else {
llvm_unreachable("Unknown SimpleArgument type!");
}
@@ -618,6 +626,10 @@
virtual void writeValueImpl(raw_ostream ) const {
OS << "OS << Val;\n";
}
+// Assumed to receive a parameter: raw_ostream OS.
+virtual void writeDumpImpl(raw_ostream ) const {
+ OS << " OS << \" \" << Val;\n";
+}
public:
VariadicArgument(const Record , StringRef Attr, std::string T)
@@ -744,7 +756,22 @@
void writeDump(raw_ostream ) const override {
OS << "for (const auto : SA->" << RangeName << "())\n";
- OS << " OS << \" \" << Val;\n";
+ writeDumpImpl(OS);
+}
+ };
+
+ class VariadicParamIdxArgument : public VariadicArgument {
+ public:
+VariadicParamIdxArgument(const Record , StringRef Attr)
+: VariadicArgument(Arg, Attr, "ParamIdx") {}
+
+ public:
+void writeValueImpl(raw_ostream ) const override {
+ OS << "OS << Val.getSourceIndex();\n";
+}
+
+void writeDumpImpl(raw_ostream ) const override {
+ OS << " OS << \" \" << Val.getSourceIndex();\n";
}
};
@@ -1247,6 +1274,10 @@
Ptr = llvm::make_unique(Arg, Attr);
else if (ArgName == "VariadicExprArgument")
Ptr = llvm::make_unique(Arg, Attr);
+ else if (ArgName == "VariadicParamIdxArgument")
+Ptr = llvm::make_unique(Arg, Attr);
+ else if (ArgName == "ParamIdxArgument")
+Ptr = llvm::make_unique(Arg, Attr, "ParamIdx");
else if (ArgName == "VersionArgument")
Ptr = llvm::make_unique(Arg, Attr);
Index: trunk/test/Sema/error-type-safety.cpp
===
--- trunk/test/Sema/error-type-safety.cpp
+++ trunk/test/Sema/error-type-safety.cpp
@@ -3,21 +3,50 @@
#define INT_TAG 42
static const int test_in
- __attribute__((type_tag_for_datatype(test, int))) = INT_TAG;
+__attribute__((type_tag_for_datatype(test, int))) = INT_TAG;
// Argument index: 1, Type tag index: 2
void test_bounds_index(...)
-
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added a comment. Can you regenerate the patch using the same paths as https://reviews.llvm.org/D43248?id=136811? When I try to do a diff between what was accepted & committed and the current patch, Phabricator gets confused because the paths are too different from one another. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny updated this revision to Diff 137964.
jdenny added a comment.
This commit was reverted by r326862 due to:
https://bugs.llvm.org/show_bug.cgi?id=36620
This revision includes a new test case and a fix.
While the difference from the last revision is small, it's not trivial, so
another review is probably worthwhile. The main difference is two new
ParamIdx member functions: serialize and deserialize. In
ClangAttrEmitter.cpp, these functions enable significant simplifications and
facilitate the bug fix.
https://reviews.llvm.org/D43248
Files:
include/clang/AST/Attr.h
include/clang/Basic/Attr.td
lib/AST/ExprConstant.cpp
lib/CodeGen/CGCall.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
test/CodeGenCXX/alloc-size.cpp
test/Frontend/ast-attr.cpp
test/Misc/ast-dump-attr.cpp
test/Sema/attr-ownership.cpp
test/Sema/attr-print.cpp
test/Sema/error-type-safety.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -104,6 +104,7 @@
.Case("Expr *", "Record.readExpr()")
.Case("IdentifierInfo *", "Record.getIdentifierInfo()")
.Case("StringRef", "Record.readString()")
+.Case("ParamIdx", "ParamIdx::deserialize(Record.readInt())")
.Default("Record.readInt()");
}
@@ -122,6 +123,7 @@
.Case("Expr *", "AddStmt(" + std::string(name) + ");\n")
.Case("IdentifierInfo *", "AddIdentifierRef(" + std::string(name) + ");\n")
.Case("StringRef", "AddString(" + std::string(name) + ");\n")
+.Case("ParamIdx", "push_back(" + std::string(name) + ".serialize());\n")
.Default("push_back(" + std::string(name) + ");\n");
}
@@ -302,9 +304,8 @@
std::string getIsOmitted() const override {
if (type == "IdentifierInfo *")
return "!get" + getUpperName().str() + "()";
- // FIXME: Do this declaratively in Attr.td.
- if (getAttrName() == "AllocSize")
-return "0 == get" + getUpperName().str() + "()";
+ if (type == "ParamIdx")
+return "!get" + getUpperName().str() + "().isValid()";
return "false";
}
@@ -319,6 +320,8 @@
<< "()->getName() : \"\") << \"";
else if (type == "TypeSourceInfo *")
OS << "\" << get" << getUpperName() << "().getAsString() << \"";
+ else if (type == "ParamIdx")
+OS << "\" << get" << getUpperName() << "().getSourceIndex() << \"";
else
OS << "\" << get" << getUpperName() << "() << \"";
}
@@ -341,6 +344,11 @@
<< getUpperName() << "\";\n";
} else if (type == "int" || type == "unsigned") {
OS << "OS << \" \" << SA->get" << getUpperName() << "();\n";
+ } else if (type == "ParamIdx") {
+if (isOptional())
+ OS << "if (SA->get" << getUpperName() << "().isValid())\n ";
+OS << "OS << \" \" << SA->get" << getUpperName()
+ << "().getSourceIndex();\n";
} else {
llvm_unreachable("Unknown SimpleArgument type!");
}
@@ -618,6 +626,10 @@
virtual void writeValueImpl(raw_ostream ) const {
OS << "OS << Val;\n";
}
+// Assumed to receive a parameter: raw_ostream OS.
+virtual void writeDumpImpl(raw_ostream ) const {
+ OS << " OS << \" \" << Val;\n";
+}
public:
VariadicArgument(const Record , StringRef Attr, std::string T)
@@ -744,7 +756,22 @@
void writeDump(raw_ostream ) const override {
OS << "for (const auto : SA->" << RangeName << "())\n";
- OS << " OS << \" \" << Val;\n";
+ writeDumpImpl(OS);
+}
+ };
+
+ class VariadicParamIdxArgument : public VariadicArgument {
+ public:
+VariadicParamIdxArgument(const Record , StringRef Attr)
+: VariadicArgument(Arg, Attr, "ParamIdx") {}
+
+ public:
+void writeValueImpl(raw_ostream ) const override {
+ OS << "OS << Val.getSourceIndex();\n";
+}
+
+void writeDumpImpl(raw_ostream ) const override {
+ OS << " OS << \" \" << Val.getSourceIndex();\n";
}
};
@@ -1247,6 +1274,10 @@
Ptr = llvm::make_unique(Arg, Attr);
else if (ArgName == "VariadicExprArgument")
Ptr = llvm::make_unique(Arg, Attr);
+ else if (ArgName == "VariadicParamIdxArgument")
+Ptr = llvm::make_unique(Arg, Attr);
+ else if (ArgName == "ParamIdxArgument")
+Ptr = llvm::make_unique(Arg, Attr, "ParamIdx");
else if (ArgName == "VersionArgument")
Ptr = llvm::make_unique(Arg, Attr);
Index: test/Sema/error-type-safety.cpp
===
--- test/Sema/error-type-safety.cpp
+++ test/Sema/error-type-safety.cpp
@@ -3,21 +3,50 @@
#define INT_TAG 42
static const int
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326602: [Attr] Fix parameter indexing for several attributes
(authored by jdenny, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43248?vs=136624=136811#toc
Repository:
rL LLVM
https://reviews.llvm.org/D43248
Files:
cfe/trunk/include/clang/AST/Attr.h
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
cfe/trunk/test/CodeGenCXX/alloc-size.cpp
cfe/trunk/test/Misc/ast-dump-attr.cpp
cfe/trunk/test/Sema/attr-ownership.cpp
cfe/trunk/test/Sema/attr-print.cpp
cfe/trunk/test/Sema/error-type-safety.cpp
cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
Index: cfe/trunk/include/clang/AST/Attr.h
===
--- cfe/trunk/include/clang/AST/Attr.h
+++ cfe/trunk/include/clang/AST/Attr.h
@@ -195,6 +195,120 @@
}
};
+/// A single parameter index whose accessors require each use to make explicit
+/// the parameter index encoding needed.
+class ParamIdx {
+ // Idx is exposed only via accessors that specify specific encodings.
+ unsigned Idx : 30;
+ unsigned HasThis : 1;
+ unsigned IsValid : 1;
+
+ void assertComparable(const ParamIdx ) const {
+assert(isValid() && I.isValid() &&
+ "ParamIdx must be valid to be compared");
+// It's possible to compare indices from separate functions, but so far
+// it's not proven useful. Moreover, it might be confusing because a
+// comparison on the results of getASTIndex might be inconsistent with a
+// comparison on the ParamIdx objects themselves.
+assert(HasThis == I.HasThis &&
+ "ParamIdx must be for the same function to be compared");
+ }
+
+public:
+ /// Construct an invalid parameter index (\c isValid returns false and
+ /// accessors fail an assert).
+ ParamIdx() : Idx(0), HasThis(false), IsValid(false) {}
+
+ /// \param Idx is the parameter index as it is normally specified in
+ /// attributes in the source: one-origin including any C++ implicit this
+ /// parameter.
+ ///
+ /// \param D is the declaration containing the parameters. It is used to
+ /// determine if there is a C++ implicit this parameter.
+ ParamIdx(unsigned Idx, const Decl *D)
+ : Idx(Idx), HasThis(false), IsValid(true) {
+if (const auto *FD = dyn_cast(D))
+ HasThis = FD->isCXXInstanceMember();
+ }
+
+ /// \param Idx is the parameter index as it is normally specified in
+ /// attributes in the source: one-origin including any C++ implicit this
+ /// parameter.
+ ///
+ /// \param HasThis specifies whether the function has a C++ implicit this
+ /// parameter.
+ ParamIdx(unsigned Idx, bool HasThis)
+ : Idx(Idx), HasThis(HasThis), IsValid(true) {}
+
+ /// Is this parameter index valid?
+ bool isValid() const { return IsValid; }
+
+ /// Is there a C++ implicit this parameter?
+ bool hasThis() const {
+assert(isValid() && "ParamIdx must be valid");
+return HasThis;
+ }
+
+ /// Get the parameter index as it would normally be encoded for attributes at
+ /// the source level of representation: one-origin including any C++ implicit
+ /// this parameter.
+ ///
+ /// This encoding thus makes sense for diagnostics, pretty printing, and
+ /// constructing new attributes from a source-like specification.
+ unsigned getSourceIndex() const {
+assert(isValid() && "ParamIdx must be valid");
+return Idx;
+ }
+
+ /// Get the parameter index as it would normally be encoded at the AST level
+ /// of representation: zero-origin not including any C++ implicit this
+ /// parameter.
+ ///
+ /// This is the encoding primarily used in Sema. However, in diagnostics,
+ /// Sema uses \c getSourceIndex instead.
+ unsigned getASTIndex() const {
+assert(isValid() && "ParamIdx must be valid");
+assert(Idx >= 1 + HasThis &&
+ "stored index must be base-1 and not specify C++ implicit this");
+return Idx - 1 - HasThis;
+ }
+
+ /// Get the parameter index as it would normally be encoded at the LLVM level
+ /// of representation: zero-origin including any C++ implicit this parameter.
+ ///
+ /// This is the encoding primarily used in CodeGen.
+ unsigned getLLVMIndex() const {
+assert(isValid() && "ParamIdx must be valid");
+assert(Idx >= 1 && "stored index must be base-1");
+return Idx - 1;
+ }
+
+ bool operator==(const ParamIdx ) const {
+assertComparable(I);
+return Idx == I.Idx;
+ }
+ bool operator!=(const ParamIdx ) const {
+assertComparable(I);
+return Idx != I.Idx;
+ }
+ bool operator<(const
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added a comment. In https://reviews.llvm.org/D43248#1025495, @jdenny wrote: > Aaron, thanks for the review. I've applied your suggestions and am ready to > commit. You're correct that we have a lot of freedom with the commit message, but the important piece is in describing what's changed and why (if it's not immediately obvious). Both commit messages look good to me. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added a comment. Aaron, thanks for the review. I've applied your suggestions and am ready to commit. I've noticed a variety of styles in commit logs, I've read the coding standards, and it seems there's a lot of freedom here. Below are the two commit logs I'm planning to use. Please let me know if you have any objections. Thanks. [Attr] Fix parameter indexing for several attributes The patch fixes a number of bugs related to parameter indexing in attributes: * Parameter indices in some attributes (argument_with_type_tag, pointer_with_type_tag, nonnull, ownership_takes, ownership_holds, and ownership_returns) are specified in source as one-origin including any C++ implicit this parameter, were stored as zero-origin excluding any this parameter, and were erroneously printing (-ast-print) and confusingly dumping (-ast-dump) as the stored values. * For alloc_size, the C++ implicit this parameter was not subtracted correctly in Sema, leading to assert failures or to silent failures of __builtin_object_size to compute a value. * For argument_with_type_tag, pointer_with_type_tag, and ownership_returns, the C++ implicit this parameter was not added back to parameter indices in some diagnostics. This patch fixes the above bugs and aims to prevent similar bugs in the future by introducing careful mechanisms for handling parameter indices in attributes. ParamIdx stores a parameter index and is designed to hide the stored encoding while providing accessors that require each use (such as printing) to make explicit the encoding that is needed. Attribute declarations declare parameter index arguments as [Variadic]ParamIdxArgument, which are exposed as ParamIdx[*]. This patch rewrites all attribute arguments that are processed by checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp to be declared as [Variadic]ParamIdxArgument. The only exception is xray_log_args's argument, which is encoded as a count not an index. Differential Revision: https://reviews.llvm.org/D43248 For the test/Sema/attr-ownership.c change: [Attr] Use -fsyntax-only in test Suggested at: https://reviews.llvm.org/D43248 https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added inline comments. Comment at: test/Sema/attr-ownership.cpp:1 +// RUN: %clang_cc1 %s -verify + jdenny wrote: > aaron.ballman wrote: > > Please pass `-fsyntax-only` as well. > Sure. Would you like me to change test/Sema/attr-ownership.c while we're > thinking about it? Because it's unrelated to this patch, that can be done in a separate commit (no review required, you can just make the change and commit it). https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added inline comments. Comment at: test/Sema/attr-ownership.cpp:1 +// RUN: %clang_cc1 %s -verify + aaron.ballman wrote: > Please pass `-fsyntax-only` as well. Sure. Would you like me to change test/Sema/attr-ownership.c while we're thinking about it? https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from two minor nits, this LGTM. Thank you for working on it! Comment at: test/Sema/attr-ownership.cpp:1 +// RUN: %clang_cc1 %s -verify + Please pass `-fsyntax-only` as well. Comment at: utils/TableGen/ClangAttrEmitter.cpp:763 + << "assert(HasThis == Idx.hasThis() && " +"\"HasThis must be consistent\");\n" + << " }\n" Given how oddly this is wrapped, might as well make this a stream argument rather than a string literal concatenation. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added inline comments.
Comment at: include/clang/AST/Attr.h:206
+
+ void cmpable(const ParamIdx ) const {
+assert(isValid() && I.isValid() &&
aaron.ballman wrote:
> jdenny wrote:
> > aaron.ballman wrote:
> > > The name here can be improved. How about `checkInvariants()`? Might as
> > > well make this inline while you're at it.
> > Sure, I can change the name.
> >
> > It's inside the class, so specifying inline is against the LLVM coding
> > standards, right?
> Derp, you're correct, it's already implicitly inline. Ignore that part of the
> suggestion.
> The name here can be improved. How about checkInvariants()?
I went with assertComparable because, in my view, this is not so much a class
invariant as it is an assertion about correct usage of comparison operators.
But I'm not married to it.
https://reviews.llvm.org/D43248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny updated this revision to Diff 136624.
jdenny marked 23 inline comments as done.
jdenny edited the summary of this revision.
jdenny added a comment.
This update should address all outstanding comments.
https://reviews.llvm.org/D43248
Files:
include/clang/AST/Attr.h
include/clang/Basic/Attr.td
lib/AST/ExprConstant.cpp
lib/CodeGen/CGCall.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
test/CodeGenCXX/alloc-size.cpp
test/Misc/ast-dump-attr.cpp
test/Sema/attr-ownership.cpp
test/Sema/attr-print.cpp
test/Sema/error-type-safety.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -302,9 +302,6 @@
std::string getIsOmitted() const override {
if (type == "IdentifierInfo *")
return "!get" + getUpperName().str() + "()";
- // FIXME: Do this declaratively in Attr.td.
- if (getAttrName() == "AllocSize")
-return "0 == get" + getUpperName().str() + "()";
return "false";
}
@@ -748,6 +745,138 @@
}
};
+ class VariadicParamIdxArgument : public VariadicArgument {
+ public:
+VariadicParamIdxArgument(const Record , StringRef Attr)
+: VariadicArgument(Arg, Attr, "ParamIdx") {}
+
+ public:
+void writeCtorBody(raw_ostream ) const override {
+ VariadicArgument::writeCtorBody(OS);
+ OS << "#ifndef NDEBUG\n"
+ << "if (" << getLowerName() << "_size()) {\n"
+ << " bool HasThis = " << getLowerName()
+ << "_begin()->hasThis();\n"
+ << " for (const auto Idx : " << getLowerName() << "()) {\n"
+ << "assert(Idx.isValid() && \"ParamIdx must be valid\");\n"
+ << "assert(HasThis == Idx.hasThis() && "
+"\"HasThis must be consistent\");\n"
+ << " }\n"
+ << "}\n"
+ << "#endif\n";
+}
+
+void writePCHReadDecls(raw_ostream ) const override {
+ OS << "unsigned " << getUpperName() << "Size = Record.readInt();\n";
+ OS << "bool " << getUpperName() << "HasThis = " << getUpperName()
+ << "Size ? Record.readInt() : false;\n";
+ OS << "SmallVector " << getUpperName() << ";\n"
+ << "" << getUpperName() << ".reserve(" << getUpperName()
+ << "Size);\n"
+ << "for (unsigned i = 0; i != " << getUpperName()
+ << "Size; ++i) {\n"
+ << " " << getUpperName()
+ << ".push_back(ParamIdx(Record.readInt(), " << getUpperName()
+ << "HasThis));\n"
+ << "}\n";
+}
+
+void writePCHReadArgs(raw_ostream ) const override {
+ OS << getUpperName() << ".data(), " << getUpperName() << "Size";
+}
+
+void writePCHWrite(raw_ostream ) const override {
+ OS << "Record.push_back(SA->" << getLowerName() << "_size());\n";
+ OS << "if (SA->" << getLowerName() << "_size())\n"
+ << " Record.push_back(SA->" << getLowerName()
+ << "_begin()->hasThis());\n";
+ OS << "for (auto Idx : SA->" << getLowerName() << "())\n"
+ << " Record.push_back(Idx.getSourceIndex());\n";
+}
+
+void writeValueImpl(raw_ostream ) const override {
+ OS << "OS << Val.getSourceIndex();\n";
+}
+
+void writeDump(raw_ostream ) const override {
+ OS << "for (auto Idx : SA->" << getLowerName() << "())\n";
+ OS << " OS << \" \" << Idx.getSourceIndex();\n";
+}
+ };
+
+ class ParamIdxArgument : public Argument {
+std::string IdxName;
+
+ public:
+ParamIdxArgument(const Record , StringRef Attr)
+: Argument(Arg, Attr), IdxName(getUpperName()) {}
+
+void writeDeclarations(raw_ostream ) const override {
+ OS << "ParamIdx " << IdxName << ";\n";
+}
+
+void writeAccessors(raw_ostream ) const override {
+ OS << "\n"
+ << " ParamIdx " << getLowerName() << "() const {"
+ << " return " << IdxName << "; }\n";
+}
+
+void writeCtorParameters(raw_ostream ) const override {
+ OS << "ParamIdx " << IdxName;
+}
+
+void writeCloneArgs(raw_ostream ) const override { OS << IdxName; }
+
+void writeTemplateInstantiationArgs(raw_ostream ) const override {
+ OS << "A->" << getLowerName() << "()";
+}
+
+void writeImplicitCtorArgs(raw_ostream ) const override {
+ OS << IdxName;
+}
+
+void writeCtorInitializers(raw_ostream ) const override {
+ OS << IdxName << "(" << IdxName << ")";
+}
+
+void writeCtorDefaultInitializers(raw_ostream ) const override {
+ OS << IdxName << "()";
+}
+
+void writePCHReadDecls(raw_ostream ) const override {
+ OS << "unsigned
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny marked 3 inline comments as done. jdenny added inline comments. Comment at: include/clang/Basic/Attr.td:172-174 + // Whether the C++ implicit this parameter is allowed. Users that construct + // attributes from the source code use this information when validating + // parameter indices. aaron.ballman wrote: > jdenny wrote: > > aaron.ballman wrote: > > > I still find the `AllowThis` parts to be hard to follow, so I want to be > > > sure I understand your design properly. Everything that uses this new > > > argument type sets `AllowsThis` to 0. As written, it sounds like setting > > > that to 0 means that the parameter index cannot be used on a C++ instance > > > method, and I know that's not the correct interpretation. Under what > > > circumstances would this be set to 1 instead? > > > > > > Looking at the existing code, the only circumstances under which > > > `checkFunctionOrMethodParameterIndex()` was being passed `true` for > > > "allow this" was for XRayLogArgs, which doesn't even use `ParamIdx`. > > > Perhaps this member isn't necessary any longer? > > > I still find the AllowThis parts to be hard to follow, so I want to be > > > sure I understand your design properly. Everything that uses this new > > > argument type sets AllowsThis to 0. As written, it sounds like setting > > > that to 0 means that the parameter index cannot be used on a C++ instance > > > method, and I know that's not the correct interpretation. > > > > Right. AllowsThis=0 means it is an error for the attribute in the source > > code to specify the C++ implicit this parameter (index 1). > > > > > Under what circumstances would this be set to 1 instead? > > > > > > Looking at the existing code, the only circumstances under which > > > checkFunctionOrMethodParameterIndex() was being passed true for "allow > > > this" was for XRayLogArgs, which doesn't even use ParamIdx. Perhaps this > > > member isn't necessary any longer? > > > > Right. I also noticed this issue, but I probably should have mentioned > > that in a comment in the source instead of just rewording the last > > paragraph of the patch summary. Sorry. > > > > I thought about removing AllowsThis, but I hesitated because I had already > > implemented it by the time I noticed this issue and because I assumed there > > must be some reason why attributes for C++ have index 1 mean the this > > parameter, so there might be some attribute that could eventually take > > advantage of AllowsThis=1. Moreover, it makes the related argument to > > checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp clearer. > > > > I don't feel strongly either way, so I'm happy to remove it or keep it. > > Right. AllowsThis=0 means it is an error for the attribute in the source > > code to specify the C++ implicit this parameter (index 1). > > Then if we keep this functionality, I think a better identifier would be > something like `CanIndexImplicitThis` and the comments could be updated to > more clearly state what is allowed/disallowed. Then the other uses of "allow > this" can be updated to use similar terminology for clarity. > > > so there might be some attribute that could eventually take advantage of > > AllowsThis=1. Moreover, it makes the related argument to > > checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp clearer. > > My gut feeling is that this functionality isn't going to be needed all that > frequently. If we get a second case where we need it, then I'd say it might > make more sense to add it. > > Attributes that use positional arguments should hopefully be the exception, > not the rule, because those indexes are horribly fragile. > > What do you think? > My gut feeling is that this functionality isn't going to be needed all that > frequently. If we get a second case where we need it, then I'd say it might > make more sense to add it. > > Attributes that use positional arguments should hopefully be the exception, > not the rule, because those indexes are horribly fragile. > > What do you think? I'm guessing you have more experience with attributes in general, so let's go with your gut. :-) Comment at: lib/Sema/SemaDeclAttr.cpp:4549-4551 +if (ArgumentIdx.getASTIndex() >= getFunctionOrMethodNumParams(D) || +!getFunctionOrMethodParamType(D, ArgumentIdx.getASTIndex()) + ->isPointerType()) aaron.ballman wrote: > jdenny wrote: > > aaron.ballman wrote: > > > Is this formatting produced by clang-format? > > Yes. > How funky. :-) Agreed. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added inline comments.
Comment at: include/clang/AST/Attr.h:206
+
+ void cmpable(const ParamIdx ) const {
+assert(isValid() && I.isValid() &&
jdenny wrote:
> aaron.ballman wrote:
> > The name here can be improved. How about `checkInvariants()`? Might as well
> > make this inline while you're at it.
> Sure, I can change the name.
>
> It's inside the class, so specifying inline is against the LLVM coding
> standards, right?
Derp, you're correct, it's already implicitly inline. Ignore that part of the
suggestion.
Comment at: include/clang/AST/Attr.h:236
+ /// parameter.
+ ParamIdx(unsigned Idx, bool HasThis)
+ : Idx(Idx), HasThis(HasThis), IsValid(true) {}
jdenny wrote:
> aaron.ballman wrote:
> > Is this constructor used anywhere? I didn't see it being used, but I could
> > have missed something. If it's not used, go ahead and remove it.
> It's used by the deserialization code generated by ClangAttrEmitter.cpp.
That'd explain why I hadn't seen it.
Comment at: include/clang/AST/Attr.h:281-284
+ bool operator<(const ParamIdx ) const { cmpable(I); return Idx < I.Idx; }
+ bool operator>(const ParamIdx ) const { cmpable(I); return Idx > I.Idx; }
+ bool operator<=(const ParamIdx ) const { cmpable(I); return Idx <= I.Idx; }
+ bool operator>=(const ParamIdx ) const { cmpable(I); return Idx >= I.Idx; }
jdenny wrote:
> aaron.ballman wrote:
> > Are all of these operations required for the class?
> operator== and operator< are needed for sorting and finding. It seems
> strange to me not to finish the set.
I don't think it's actively harmful to do so, but I also don't think it's
really needed either. Your call.
Comment at: include/clang/Basic/Attr.td:172-174
+ // Whether the C++ implicit this parameter is allowed. Users that construct
+ // attributes from the source code use this information when validating
+ // parameter indices.
jdenny wrote:
> aaron.ballman wrote:
> > I still find the `AllowThis` parts to be hard to follow, so I want to be
> > sure I understand your design properly. Everything that uses this new
> > argument type sets `AllowsThis` to 0. As written, it sounds like setting
> > that to 0 means that the parameter index cannot be used on a C++ instance
> > method, and I know that's not the correct interpretation. Under what
> > circumstances would this be set to 1 instead?
> >
> > Looking at the existing code, the only circumstances under which
> > `checkFunctionOrMethodParameterIndex()` was being passed `true` for "allow
> > this" was for XRayLogArgs, which doesn't even use `ParamIdx`. Perhaps this
> > member isn't necessary any longer?
> > I still find the AllowThis parts to be hard to follow, so I want to be sure
> > I understand your design properly. Everything that uses this new argument
> > type sets AllowsThis to 0. As written, it sounds like setting that to 0
> > means that the parameter index cannot be used on a C++ instance method, and
> > I know that's not the correct interpretation.
>
> Right. AllowsThis=0 means it is an error for the attribute in the source code
> to specify the C++ implicit this parameter (index 1).
>
> > Under what circumstances would this be set to 1 instead?
> >
> > Looking at the existing code, the only circumstances under which
> > checkFunctionOrMethodParameterIndex() was being passed true for "allow
> > this" was for XRayLogArgs, which doesn't even use ParamIdx. Perhaps this
> > member isn't necessary any longer?
>
> Right. I also noticed this issue, but I probably should have mentioned that
> in a comment in the source instead of just rewording the last paragraph of
> the patch summary. Sorry.
>
> I thought about removing AllowsThis, but I hesitated because I had already
> implemented it by the time I noticed this issue and because I assumed there
> must be some reason why attributes for C++ have index 1 mean the this
> parameter, so there might be some attribute that could eventually take
> advantage of AllowsThis=1. Moreover, it makes the related argument to
> checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp clearer.
>
> I don't feel strongly either way, so I'm happy to remove it or keep it.
> Right. AllowsThis=0 means it is an error for the attribute in the source code
> to specify the C++ implicit this parameter (index 1).
Then if we keep this functionality, I think a better identifier would be
something like `CanIndexImplicitThis` and the comments could be updated to more
clearly state what is allowed/disallowed. Then the other uses of "allow this"
can be updated to use similar terminology for clarity.
> so there might be some attribute that could eventually take advantage of
> AllowsThis=1. Moreover, it makes the related argument to
> checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp clearer.
My gut feeling is that this
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added a comment.
In https://reviews.llvm.org/D43248#1024183, @aaron.ballman wrote:
> In https://reviews.llvm.org/D43248#1023720, @jdenny wrote:
>
> > Hi Aaron. It occurs to me now that this patch has grown rather large and,
> > in some places, a little subtle. Would it help the review if I were to
> > break it up into a patch series that introduces ParamIdx to each attribute,
> > one at a time? I'm not trying to rush you, but I hate for the review to be
> > painful for you if it doesn't have to be.
>
>
> No need to do that -- this review just takes a bit more time for me to
> complete, but it's reasonably well-factored. Thank you, though!
Sure! Thanks for the review.
Comment at: include/clang/AST/Attr.h:206
+
+ void cmpable(const ParamIdx ) const {
+assert(isValid() && I.isValid() &&
aaron.ballman wrote:
> The name here can be improved. How about `checkInvariants()`? Might as well
> make this inline while you're at it.
Sure, I can change the name.
It's inside the class, so specifying inline is against the LLVM coding
standards, right?
Comment at: include/clang/AST/Attr.h:236
+ /// parameter.
+ ParamIdx(unsigned Idx, bool HasThis)
+ : Idx(Idx), HasThis(HasThis), IsValid(true) {}
aaron.ballman wrote:
> Is this constructor used anywhere? I didn't see it being used, but I could
> have missed something. If it's not used, go ahead and remove it.
It's used by the deserialization code generated by ClangAttrEmitter.cpp.
Comment at: include/clang/AST/Attr.h:281-284
+ bool operator<(const ParamIdx ) const { cmpable(I); return Idx < I.Idx; }
+ bool operator>(const ParamIdx ) const { cmpable(I); return Idx > I.Idx; }
+ bool operator<=(const ParamIdx ) const { cmpable(I); return Idx <= I.Idx; }
+ bool operator>=(const ParamIdx ) const { cmpable(I); return Idx >= I.Idx; }
aaron.ballman wrote:
> Are all of these operations required for the class?
operator== and operator< are needed for sorting and finding. It seems strange
to me not to finish the set.
Comment at: include/clang/Basic/Attr.td:172-174
+ // Whether the C++ implicit this parameter is allowed. Users that construct
+ // attributes from the source code use this information when validating
+ // parameter indices.
aaron.ballman wrote:
> I still find the `AllowThis` parts to be hard to follow, so I want to be sure
> I understand your design properly. Everything that uses this new argument
> type sets `AllowsThis` to 0. As written, it sounds like setting that to 0
> means that the parameter index cannot be used on a C++ instance method, and I
> know that's not the correct interpretation. Under what circumstances would
> this be set to 1 instead?
>
> Looking at the existing code, the only circumstances under which
> `checkFunctionOrMethodParameterIndex()` was being passed `true` for "allow
> this" was for XRayLogArgs, which doesn't even use `ParamIdx`. Perhaps this
> member isn't necessary any longer?
> I still find the AllowThis parts to be hard to follow, so I want to be sure I
> understand your design properly. Everything that uses this new argument type
> sets AllowsThis to 0. As written, it sounds like setting that to 0 means that
> the parameter index cannot be used on a C++ instance method, and I know
> that's not the correct interpretation.
Right. AllowsThis=0 means it is an error for the attribute in the source code
to specify the C++ implicit this parameter (index 1).
> Under what circumstances would this be set to 1 instead?
>
> Looking at the existing code, the only circumstances under which
> checkFunctionOrMethodParameterIndex() was being passed true for "allow this"
> was for XRayLogArgs, which doesn't even use ParamIdx. Perhaps this member
> isn't necessary any longer?
Right. I also noticed this issue, but I probably should have mentioned that in
a comment in the source instead of just rewording the last paragraph of the
patch summary. Sorry.
I thought about removing AllowsThis, but I hesitated because I had already
implemented it by the time I noticed this issue and because I assumed there
must be some reason why attributes for C++ have index 1 mean the this
parameter, so there might be some attribute that could eventually take
advantage of AllowsThis=1. Moreover, it makes the related argument to
checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp clearer.
I don't feel strongly either way, so I'm happy to remove it or keep it.
Comment at: lib/Sema/SemaDeclAttr.cpp:1650-1651
OwnershipAttr(AL.getLoc(), S.Context, nullptr, nullptr, 0,
-AL.getAttributeSpellingListIndex()).getOwnKind();
+AL.getAttributeSpellingListIndex())
+ .getOwnKind();
aaron.ballman wrote:
> This change looks to be unrelated to the patch?
Sorry, I think clang-format
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added a comment. In https://reviews.llvm.org/D43248#1023720, @jdenny wrote: > Hi Aaron. It occurs to me now that this patch has grown rather large and, in > some places, a little subtle. Would it help the review if I were to break it > up into a patch series that introduces ParamIdx to each attribute, one at a > time? I'm not trying to rush you, but I hate for the review to be painful > for you if it doesn't have to be. No need to do that -- this review just takes a bit more time for me to complete, but it's reasonably well-factored. Thank you, though! https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added inline comments.
Comment at: include/clang/AST/Attr.h:206
+
+ void cmpable(const ParamIdx ) const {
+assert(isValid() && I.isValid() &&
The name here can be improved. How about `checkInvariants()`? Might as well
make this inline while you're at it.
Comment at: include/clang/AST/Attr.h:236
+ /// parameter.
+ ParamIdx(unsigned Idx, bool HasThis)
+ : Idx(Idx), HasThis(HasThis), IsValid(true) {}
Is this constructor used anywhere? I didn't see it being used, but I could have
missed something. If it's not used, go ahead and remove it.
Comment at: include/clang/AST/Attr.h:267
+assert(isValid() && "ParamIdx must be valid");
+return Idx - 1 - HasThis;
+ }
Please assert that `Idx` won't wrap before doing the return.
Comment at: include/clang/AST/Attr.h:276
+assert(isValid() && "ParamIdx must be valid");
+return Idx - 1;
+ }
Likewise here.
Comment at: include/clang/AST/Attr.h:281-284
+ bool operator<(const ParamIdx ) const { cmpable(I); return Idx < I.Idx; }
+ bool operator>(const ParamIdx ) const { cmpable(I); return Idx > I.Idx; }
+ bool operator<=(const ParamIdx ) const { cmpable(I); return Idx <= I.Idx; }
+ bool operator>=(const ParamIdx ) const { cmpable(I); return Idx >= I.Idx; }
Are all of these operations required for the class?
Comment at: include/clang/Basic/Attr.td:172-174
+ // Whether the C++ implicit this parameter is allowed. Users that construct
+ // attributes from the source code use this information when validating
+ // parameter indices.
I still find the `AllowThis` parts to be hard to follow, so I want to be sure I
understand your design properly. Everything that uses this new argument type
sets `AllowsThis` to 0. As written, it sounds like setting that to 0 means that
the parameter index cannot be used on a C++ instance method, and I know that's
not the correct interpretation. Under what circumstances would this be set to 1
instead?
Looking at the existing code, the only circumstances under which
`checkFunctionOrMethodParameterIndex()` was being passed `true` for "allow
this" was for XRayLogArgs, which doesn't even use `ParamIdx`. Perhaps this
member isn't necessary any longer?
Comment at: lib/Sema/SemaChecking.cpp:2622
- for (unsigned Val : NonNull->args()) {
-if (Val >= Args.size())
+ for (ParamIdx Idx : NonNull->args()) {
+if (Idx.getASTIndex() >= Args.size())
`const ParamIdx &`
Comment at: lib/Sema/SemaChecking.cpp:10083
- for (unsigned ArgNo : NonNull->args()) {
-if (ArgNo == ParamNo) {
+ for (ParamIdx ArgNo : NonNull->args()) {
+if (ArgNo.getASTIndex() == ParamNo) {
`const ParamIdx &`
Comment at: lib/Sema/SemaChecking.cpp:12244
}
- const Expr *TypeTagExpr = ExprArgs[Attr->getTypeTagIdx()];
+ const Expr *TypeTagExpr = ExprArgs[Attr->typeTagIdx().getASTIndex()];
bool FoundWrongKind;
Hoist the AST index so you don't have to call for it twice. (Same applies
elsewhere.)
Comment at: lib/Sema/SemaDeclAttr.cpp:785-786
- const ParmVarDecl *Param = FD->getParamDecl(Idx);
- if (AllowDependentType && Param->getType()->isDependentType())
-return true;
if (!Param->getType()->isIntegerType() && !Param->getType()->isCharType()) {
Good catch about this not being needed any longer!
Comment at: lib/Sema/SemaDeclAttr.cpp:1650-1651
OwnershipAttr(AL.getLoc(), S.Context, nullptr, nullptr, 0,
-AL.getAttributeSpellingListIndex()).getOwnKind();
+AL.getAttributeSpellingListIndex())
+ .getOwnKind();
This change looks to be unrelated to the patch?
Comment at: lib/Sema/SemaDeclAttr.cpp:4549-4551
+if (ArgumentIdx.getASTIndex() >= getFunctionOrMethodNumParams(D) ||
+!getFunctionOrMethodParamType(D, ArgumentIdx.getASTIndex())
+ ->isPointerType())
Is this formatting produced by clang-format?
Comment at: lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:61
}
-for (unsigned Val : NonNull->args()) {
- if (Val >= NumArgs)
+for (ParamIdx Idx : NonNull->args()) {
+ if (Idx.getASTIndex() >= NumArgs)
`const ParamIdx &`
https://reviews.llvm.org/D43248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added a comment. Hi Aaron. It occurs to me now that this patch has grown rather large and, in some places, a little subtle. Would it help the review if I were to break it up into a patch series that introduces ParamIdx to each attribute, one at a time? I'm not trying to rush you, but I hate for the review to be painful for you if it doesn't have to be. https://reviews.llvm.org/D43248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny added inline comments.
Comment at: include/clang/AST/Attr.h:210-212
+ unsigned Idx;
+ bool HasThis;
+ bool IsValid;
aaron.ballman wrote:
> I think it might be best to mash these together using bit-fields:
> ```
> unsigned Idx : 30;
> unsigned HasThis : 1;
> unsigned IsValid : 1;
> ```
Good point. Thanks.
Comment at: include/clang/AST/Attr.h:238-243
+ ParamIdx =(const ParamIdx ) {
+Idx = I.Idx;
+HasThis = I.HasThis;
+IsValid = I.IsValid;
+return *this;
+ }
aaron.ballman wrote:
> Is this necessary? There should be an implicit copy assignment operator for
> this class, and it's a bit strange to have a copy assignment but no copy
> constructor.
Oops. Thanks.
Comment at: include/clang/AST/Attr.h:291-292
+template
+class ParamIdxItrBase : public std::iterator {
+ DerivedT () { return *static_cast(this); }
aaron.ballman wrote:
> `std::iterator` was deprecated in C++17, so you should manually expose these
> fields.
Now that sizeof(ParamIdx) == sizeof(unsigned), I can no longer find a reason to
avoid ParamIdx arrays, and so I can no longer find a good justification for the
iterator classes.
https://reviews.llvm.org/D43248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny updated this revision to Diff 136019.
jdenny marked 8 inline comments as done.
jdenny edited the summary of this revision.
jdenny added a comment.
This revision should address all issues raised.
https://reviews.llvm.org/D43248
Files:
include/clang/AST/Attr.h
include/clang/Basic/Attr.td
lib/AST/ExprConstant.cpp
lib/CodeGen/CGCall.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
test/CodeGenCXX/alloc-size.cpp
test/Misc/ast-dump-attr.cpp
test/Sema/attr-ownership.cpp
test/Sema/attr-print.cpp
test/Sema/error-type-safety.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -306,9 +306,6 @@
return "!get" + getUpperName().str() + "()";
if (type == "TypeSourceInfo *")
return "false";
- // FIXME: Do this declaratively in Attr.td.
- if (getAttrName() == "AllocSize")
-return "0 == get" + getUpperName().str() + "()";
return "false";
}
@@ -752,6 +749,152 @@
}
};
+ class VariadicParamIdxArgument : public VariadicArgument {
+bool AllowsThis;
+
+ public:
+VariadicParamIdxArgument(const Record , StringRef Attr)
+: VariadicArgument(Arg, Attr, "ParamIdx"),
+ AllowsThis(Arg.getValueAsBit("AllowsThis")) {}
+
+ public:
+void writeAccessors(raw_ostream ) const override {
+ VariadicArgument::writeAccessors(OS);
+ OS << "\n"
+ << " static bool " << getLowerName() << "_allowsThis() {"
+ << " return " << AllowsThis << "; }\n";
+}
+
+void writeCtorBody(raw_ostream ) const override {
+ VariadicArgument::writeCtorBody(OS);
+ OS << "#ifndef NDEBUG\n"
+ << "if (" << getLowerName() << "_size()) {\n"
+ << " bool HasThis = " << getLowerName()
+ << "_begin()->hasThis();\n"
+ << " for (const auto Idx : " << getLowerName() << "()) {\n"
+ << "assert(Idx.isValid() && \"ParamIdx must be valid\");\n"
+ << "assert(HasThis == Idx.hasThis() && "
+"\"HasThis must be consistent\");\n"
+ << " }\n"
+ << "}\n"
+ << "#endif\n";
+}
+
+void writePCHReadDecls(raw_ostream ) const override {
+ OS << "unsigned " << getUpperName() << "Size = Record.readInt();\n";
+ OS << "bool " << getUpperName() << "HasThis = " << getUpperName()
+ << "Size ? Record.readInt() : false;\n";
+ OS << "SmallVector " << getUpperName() << ";\n"
+ << "" << getUpperName() << ".reserve(" << getUpperName()
+ << "Size);\n"
+ << "for (unsigned i = 0; i != " << getUpperName()
+ << "Size; ++i) {\n"
+ << " " << getUpperName()
+ << ".push_back(ParamIdx(Record.readInt(), " << getUpperName()
+ << "HasThis));\n"
+ << "}\n";
+}
+
+void writePCHReadArgs(raw_ostream ) const override {
+ OS << getUpperName() << ".data(), " << getUpperName() << "Size";
+}
+
+void writePCHWrite(raw_ostream ) const override {
+ OS << "Record.push_back(SA->" << getLowerName() << "_size());\n";
+ OS << "if (SA->" << getLowerName() << "_size())\n"
+ << " Record.push_back(SA->" << getLowerName()
+ << "_begin()->hasThis());\n";
+ OS << "for (auto Idx : SA->" << getLowerName() << "())\n"
+ << " Record.push_back(Idx.getSourceIndex());\n";
+}
+
+void writeValueImpl(raw_ostream ) const override {
+ OS << "OS << Val.getSourceIndex();\n";
+}
+
+void writeDump(raw_ostream ) const override {
+ OS << "for (auto Idx : SA->" << getLowerName() << "())\n";
+ OS << " OS << \" \" << Idx.getSourceIndex();\n";
+}
+ };
+
+ class ParamIdxArgument : public Argument {
+bool AllowsThis;
+std::string IdxName;
+
+ public:
+ParamIdxArgument(const Record , StringRef Attr)
+: Argument(Arg, Attr), AllowsThis(Arg.getValueAsBit("AllowsThis")),
+ IdxName(getUpperName()) {}
+
+void writeDeclarations(raw_ostream ) const override {
+ OS << "ParamIdx " << IdxName << ";\n";
+}
+
+void writeAccessors(raw_ostream ) const override {
+ OS << "\n"
+ << " static bool " << getLowerName() << "_allowsThis() {"
+ << " return " << AllowsThis << "; }\n";
+ OS << " ParamIdx " << getLowerName() << "() const {"
+ << " return " << IdxName << "; }\n";
+}
+
+void writeCtorParameters(raw_ostream ) const override {
+ OS << "ParamIdx " << IdxName;
+}
+
+void writeCloneArgs(raw_ostream ) const override { OS << IdxName; }
+
+void
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
aaron.ballman added a comment.
(Review is incomplete, but here are some initial comments.)
Comment at: include/clang/AST/Attr.h:210-212
+ unsigned Idx;
+ bool HasThis;
+ bool IsValid;
I think it might be best to mash these together using bit-fields:
```
unsigned Idx : 30;
unsigned HasThis : 1;
unsigned IsValid : 1;
```
Comment at: include/clang/AST/Attr.h:218
+ ParamIdx() : Idx(0), HasThis(false), IsValid(false) {}
+ /// \param Idx is the parameter index as it is normally specified in
+ /// attributes in the source: one-origin including any C++ implicit this
Add some newlines between the various declarations in the class -- everything
is condensed a bit too much without it.
Comment at: include/clang/AST/Attr.h:225
+ ParamIdx(unsigned Idx, const Decl *D) : Idx(Idx), IsValid(true) {
+if (const FunctionDecl *FD = dyn_cast(D))
+ HasThis = FD->isCXXInstanceMember();
`const auto *`
Comment at: include/clang/AST/Attr.h:227-228
+ HasThis = FD->isCXXInstanceMember();
+else
+ HasThis = false;
+ }
Might as well move this into the member initializer.
Comment at: include/clang/AST/Attr.h:238-243
+ ParamIdx =(const ParamIdx ) {
+Idx = I.Idx;
+HasThis = I.HasThis;
+IsValid = I.IsValid;
+return *this;
+ }
Is this necessary? There should be an implicit copy assignment operator for
this class, and it's a bit strange to have a copy assignment but no copy
constructor.
Comment at: include/clang/AST/Attr.h:264
+ /// constructing new attributes from a source-like specification.
+ unsigned atSrc() const {
+assert(isValid() && "ParamIdx must be valid");
I'd prefer more clear names, like "getSourceIndex()" and "getASTIndex()".
Comment at: include/clang/AST/Attr.h:282
+ /// This is the encoding primarily used in CodeGen.
+ unsigned atLLVM() const {
+assert(isValid() && "ParamIdx must be valid");
Perhaps `getLLVMIndex()` for this one.
Comment at: include/clang/AST/Attr.h:291-292
+template
+class ParamIdxItrBase : public std::iterator {
+ DerivedT () { return *static_cast(this); }
`std::iterator` was deprecated in C++17, so you should manually expose these
fields.
https://reviews.llvm.org/D43248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43248: [Attr] Fix parameter indexing for attributes
jdenny updated this revision to Diff 135839.
jdenny retitled this revision from "[Attr] Fix printing of parameter indices in
attributes" to "[Attr] Fix parameter indexing for attributes".
jdenny edited the summary of this revision.
jdenny added a comment.
Herald added a subscriber: kristof.beyls.
After several attempts at strategies Aaron and I discussed, I ended up going a
different way. See the new summary for details. I believe this version
addresses all the issues Aaron raised so far.
https://reviews.llvm.org/D43248
Files:
include/clang/AST/Attr.h
include/clang/Basic/Attr.td
lib/AST/ExprConstant.cpp
lib/CodeGen/CGCall.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
test/CodeGenCXX/alloc-size.cpp
test/Misc/ast-dump-attr.cpp
test/Sema/attr-ownership.cpp
test/Sema/attr-print.cpp
test/Sema/error-type-safety.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -306,9 +306,6 @@
return "!get" + getUpperName().str() + "()";
if (type == "TypeSourceInfo *")
return "false";
- // FIXME: Do this declaratively in Attr.td.
- if (getAttrName() == "AllocSize")
-return "0 == get" + getUpperName().str() + "()";
return "false";
}
@@ -749,6 +746,192 @@
}
};
+ class VariadicParamIdxArgument : public Argument {
+std::string IdxBeginName, SizeName;
+bool AllowsThis;
+
+ public:
+VariadicParamIdxArgument(const Record , StringRef Attr)
+: Argument(Arg, Attr), IdxBeginName(getUpperName().str() + "_begin"),
+ SizeName(getUpperName().str() + "_size"),
+ AllowsThis(Arg.getValueAsBit("AllowsThis")) {}
+
+bool isVariadic() const override { return true; }
+
+void writeDeclarations(raw_ostream ) const override {
+ OS << " ParamIdxItr " << IdxBeginName << ";\n";
+ OS << " unsigned " << SizeName << ";\n";
+}
+
+ public:
+void writeAccessors(raw_ostream ) const override {
+ OS << "\n"
+ << " static bool " << getLowerName() << "_allowsThis() {"
+ << " return " << AllowsThis << "; }\n";
+ OS << " unsigned " << getLowerName() << "_size() const {"
+ << " return " << SizeName << "; }\n";
+ OS << " ParamIdxItr " << getLowerName() << "_begin() const { return "
+ << IdxBeginName << "; }\n"
+ << " ParamIdxItr " << getLowerName() << "_end() const { return "
+ << IdxBeginName << " + " << SizeName << "; }\n"
+ << " llvm::iterator_range " << getLowerName()
+ << "() const { return llvm::make_range(" << getLowerName()
+ << "_begin(), " << getLowerName() << "_end()); }\n";
+}
+
+void writeCtorParameters(raw_ostream ) const override {
+ OS << "ParamIdxItr " << IdxBeginName << ", "
+ << "unsigned " << SizeName;
+}
+
+void writeCloneArgs(raw_ostream ) const override {
+ OS << IdxBeginName << ", " << SizeName;
+}
+
+void writeTemplateInstantiationArgs(raw_ostream ) const override {
+ // This isn't elegant, but we have to go through public methods...
+ OS << "A->" << getLowerName() << "_begin(), "
+ << "A->" << getLowerName() << "_size()";
+}
+
+void writeImplicitCtorArgs(raw_ostream ) const override {
+ OS << IdxBeginName << ", " << SizeName;
+}
+
+void writeCtorInitializers(raw_ostream ) const override {
+ OS << IdxBeginName << "(), ";
+ OS << SizeName << "(" << SizeName << ")";
+}
+
+void writeCtorDefaultInitializers(raw_ostream ) const override {
+ OS << IdxBeginName << "(), " << SizeName << "(0)";
+}
+
+void writeCtorBody(raw_ostream ) const override {
+ OS << "unsigned *" << getUpperName()
+ << "_array = new (Ctx, 16) unsigned[" << SizeName << "];\n";
+ OS << "std::copy((ParamIdxAtSrcItr)" << IdxBeginName
+ << ", (ParamIdxAtSrcItr)" << IdxBeginName << " + " << SizeName << ", "
+ << getUpperName() << "_array);\n";
+ OS << "this->" << IdxBeginName << " = ParamIdxItr(" << getUpperName()
+ << "_array, " << IdxBeginName << ".hasThis());\n";
+}
+
+void writePCHReadDecls(raw_ostream ) const override {
+ OS << "unsigned " << SizeName << " = Record.readInt();\n";
+ OS << "SmallVector " << getUpperName() << "_vec;\n"
+ << "" << getUpperName() << "_vec.reserve(" << SizeName << ");\n"
+ << "for (unsigned i = 0; i != " << SizeName << "; ++i)\n"
+ << " " << getUpperName() << "_vec.push_back(Record.readInt());\n";
+ OS << "bool " << getUpperName() << "HasThis = Record.readInt();\n";
+ OS << "
