I suspect you're right about that. Hopefully it would be easy enough.
Unfortunately, SourceLocation doesn't have an operator<, which kind of stinks.
I suspect that it'll be a non-trivial thing to do, since it is actually
possible to have attributes added with empty source-locations in some
On Fri, Aug 3, 2018 at 10:09 AM, Keane, Erich wrote:
>> As a possible idea (that may or may not work): the Attr object itself has a
>> SourceRange on it; perhaps a solution is to keep the > attributes in sorted
>> order within DeclBase::addAttr() based on the SourceRanges passed in?
>
>
> As a possible idea (that may or may not work): the Attr object itself has a
> SourceRange on it; perhaps a solution is to keep the > attributes in sorted
> order within DeclBase::addAttr() based on the SourceRanges passed in?
Interestingly, I think I came up with that idea in a comment on
On Fri, Aug 3, 2018 at 8:53 AM, Erich Keane via Phabricator
wrote:
> erichkeane added a comment.
>
> In https://reviews.llvm.org/D48100#1186654, @Meinersbur wrote:
>
>> I have two approaches to tackle the wrong marker order:
>> https://reviews.llvm.org/D50215 and https://reviews.llvm.org/D50216.
erichkeane added a comment.
In https://reviews.llvm.org/D48100#1186654, @Meinersbur wrote:
> I have two approaches to tackle the wrong marker order:
> https://reviews.llvm.org/D50215 and https://reviews.llvm.org/D50216. IMHO
> both are too invasive to be justified for the small issue.
I
Meinersbur added a comment.
I have two approaches to tackle the wrong marker order:
https://reviews.llvm.org/D50215 and https://reviews.llvm.org/D50216. IMHO both
are too invasive to be justified for the small issue.
Repository:
rL LLVM
https://reviews.llvm.org/D48100
This revision was automatically updated to reflect the committed changes.
Closed by commit rL338800: Append new attributes to the end of an
AttributeList. (authored by Meinersbur, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D48100
This revision was automatically updated to reflect the committed changes.
Closed by commit rC338800: Append new attributes to the end of an
AttributeList. (authored by Meinersbur, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48100?vs=158639=158880#toc
Repository:
rC
aaron.ballman accepted this revision.
aaron.ballman added a comment.
I am still okay with this, and agree that the ordering issues are a separate
thing to tackle.
Repository:
rC Clang
https://reviews.llvm.org/D48100
___
cfe-commits mailing list
erichkeane added a comment.
In https://reviews.llvm.org/D48100#1185189, @hfinkel wrote:
> Assuming that @aaron.ballman is still okay with this, I think that we should
> move forward. @erichkeane, I suspect that fixing the ordering problems, which
> were often arbitrary before and are still so,
hfinkel accepted this revision.
hfinkel added a comment.
Assuming that @aaron.ballman is still okay with this, I think that we should
move forward. @erichkeane, I suspect that fixing the ordering problems, which
were often arbitrary before and are still so, is a larger change that we'd want
to
Meinersbur updated this revision to Diff 158639.
Meinersbur marked an inline comment as done.
Meinersbur added a comment.
- Remove TODOs about the attribute order
Repository:
rC Clang
https://reviews.llvm.org/D48100
Files:
include/clang/Sema/ParsedAttr.h
lib/AST/ItaniumMangle.cpp
Meinersbur marked an inline comment as done.
Meinersbur added inline comments.
Comment at: test/Sema/attr-ownership.c:22
void f15(int, int)
- __attribute__((ownership_returns(foo, 1))) // expected-note {{declared with
index 1 here}}
- __attribute__((ownership_returns(foo,
erichkeane added inline comments.
Comment at: test/Sema/attr-ownership.c:22
void f15(int, int)
- __attribute__((ownership_returns(foo, 1))) // expected-note {{declared with
index 1 here}}
- __attribute__((ownership_returns(foo, 2))); // expected-error
{{'ownership_returns'
Meinersbur added a comment.
I am unsure how to proceed. Commit since already accepted? Wait for
reconfirmation? Open new differential?
Repository:
rC Clang
https://reviews.llvm.org/D48100
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Meinersbur reopened this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jrtc27.
Reopen after revert (and to be able to update the diff)
Repository:
rC Clang
https://reviews.llvm.org/D48100
Meinersbur updated this revision to Diff 158610.
Meinersbur added a comment.
Rebase after de-listifying in r336945
Repository:
rC Clang
https://reviews.llvm.org/D48100
Files:
include/clang/Sema/ParsedAttr.h
lib/AST/ItaniumMangle.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Parse/ParseObjc.cpp
Meinersbur added a comment.
In https://reviews.llvm.org/D48100#1142866, @erichkeane wrote:
> I'm currently attempting to remove the AttributeList's linked-listness.
Thank you. This should also resolve the non-optimal asymptotic execution time
to append new attributes at the end of the list.
erichkeane added a comment.
In https://reviews.llvm.org/D48100#1142842, @rsmith wrote:
> This patch appears to have at least caused us to process attributes too many
> times in some cases. For example:
>
> __attribute__((noreturn,noinline,unused)) void f();
>
>
> before your patch gives this
Meinersbur added inline comments.
Comment at: test/Sema/attr-micromips.c:9
-__attribute__((micromips,mips16)) void foo5(); // expected-error
{{'micromips' and 'mips16' attributes are not compatible}} \
+__attribute__((micromips,mips16)) void foo5(); // expected-error
rsmith added a comment.
This patch appears to have at least caused us to process attributes too many
times in some cases. For example:
__attribute__((noreturn,noinline,unused)) void f();
before your patch gives this with `clang -Xclang -ast-dump`:
`-FunctionDecl 0xccef1e0 <:1:1, col:50>
erichkeane added inline comments.
Comment at: test/Sema/attr-target-mv.c:98
int __attribute__((target("sse4.2"))) diff_cc(void);
-// expected-error@+1 {{multiversioned function declaration has a different
calling convention}}
+// expected-error@+1 {{attribute 'target'
echristo added subscribers: dlj, echristo.
echristo added a comment.
I've added a couple of inline comments here - between this and the comments in
the post-commit review from dlj it seems like we might want to revert this for
now and figure out the best way forward.
Thanks!
-eric
This revision was automatically updated to reflect the committed changes.
Closed by commit rC335084: Append new attributes to the end of an
AttributeList. (authored by Meinersbur, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48100?vs=151378=151998#toc
Repository:
rC
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, thank you for this!
Repository:
rC Clang
https://reviews.llvm.org/D48100
___
cfe-commits mailing list
Meinersbur marked an inline comment as done.
Meinersbur added a comment.
In https://reviews.llvm.org/D48100#1132208, @aaron.ballman wrote:
> Can you also simplify `hasSameOverloadableAttrs()` in ASTReaderDecl.cpp
> similar to what you did in SemaOverload.cpp (the copy seems spurious)?
Changed
Meinersbur updated this revision to Diff 151378.
Meinersbur marked an inline comment as done.
Meinersbur added a comment.
- Remove FIXME
- Add comment about O(n^2) execution time when adding attributes
- Do not store enable_if attributes in temporary list
Repository:
rC Clang
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.
Thank you for working on this odd detail of attributes!
Can you also simplify `hasSameOverloadableAttrs()` in ASTReaderDecl.cpp similar
to what you did in SemaOverload.cpp (the copy seems spurious)?
Meinersbur marked 4 inline comments as done.
Meinersbur added inline comments.
Comment at: lib/AST/ItaniumMangle.cpp:710-711
Out << "Ua9enable_ifI";
// FIXME: specific_attr_iterator iterates in reverse order. Fix that and
use
// it here.
+for
Meinersbur updated this revision to Diff 151209.
Meinersbur added a comment.
- Remove obsolete comment
- Refactor getOrderedEnableIfAttrs
Repository:
rC Clang
https://reviews.llvm.org/D48100
Files:
include/clang/Sema/AttributeList.h
lib/AST/ItaniumMangle.cpp
lib/Parse/ParseDecl.cpp
Meinersbur added inline comments.
Comment at: lib/Sema/SemaOverload.cpp:6194
static SmallVector
getOrderedEnableIfAttrs(const FunctionDecl *Function) {
SmallVector Result;
nicholas wrote:
> This function shouldn't be necessary any more. All it's doing now
nicholas added inline comments.
Comment at: lib/Sema/SemaOverload.cpp:6194
static SmallVector
getOrderedEnableIfAttrs(const FunctionDecl *Function) {
SmallVector Result;
This function shouldn't be necessary any more. All it's doing now is making an
Meinersbur created this revision.
Meinersbur added reviewers: hfinkel, TylerNowicki, ABataev, thakis, rjmccall,
george.burgess.iv, nicholas, nlewycky.
Herald added subscribers: atanasyan, zzheng.
... instead of prepending it at the beginning (the original behavior since
implemented in r122535
33 matches
Mail list logo
