aaron.ballman added a reviewer: rjmccall.
aaron.ballman added a subscriber: rjmccall.
aaron.ballman added a comment.
You should add some frontend Sema tests for the attribute. The usual tests are:
correct usage without diagnostics (as both a declaration attribute and a type
attribute), applying the attribute to the wrong subject, passing arguments to
the attribute when it doesn't accept any.
Adding @rjmccall to see if he spots any Darwin-specific concerns, but the
frontend side of things looks reasonable to me aside from a few nits.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:2258
+ let Content = [{
+On Linux ARM64 targets, this attribute changes the calling convention of
+a function to match the default convention used on Apple ARM64. This
----------------
aguinet wrote:
> aaron.ballman wrote:
> > Adding more details about what that ABI actually is, or linking to a place
> > where the user can learn more on their own, would be appreciated.
> Do you know if we can we put http links into this documentation? Would this
> render correctly in the final clang documentation?
Yup! The attribute documentation content bits form a .rst file that gets
compiled by Sphinx, so you can use any Sphinx markup you'd like
(https://www.sphinx-doc.org/en/master/usage/restructuredtext/index.html).
================
Comment at: clang/include/clang/Basic/Specifiers.h:269
+ // clang-format off
/// CallingConv - Specifies the calling convention that a function uses.
----------------
aguinet wrote:
> aaron.ballman wrote:
> > My personal opinion is that these formatting markers are noise. However,
> > there's a fair number of enumerations this could cover and disabling the
> > format behavior may be reasonable.
> >
> > I think I'd rather see the formatting comments removed from this patch.
> > They can be added in a different commit but perhaps the file can be
> > restructured so that we don't feel the need to sprinkle so many comments
> > around.
> I don't have strong opinions about this, but so we agree that `clang-format`
> won't be happy about this patch but we're fine with it?
Yup, that's fine -- the linter is already noisy in these sort of areas as it
stands, so this won't add any new noise we're not already used to seeing.
Reviewers are usually pretty good about asking for specific instances of
clang-format complaints to be resolved if they're important.
================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:70
+ // Returns true if AArch64 Darwin ABI is explicitly used.
+ const bool IsCtorOrDtor = (isa<CXXConstructorDecl>(GD.getDecl()) ||
+ (isa<CXXDestructorDecl>(GD.getDecl()) &&
----------------
aguinet wrote:
> aaron.ballman wrote:
> > We don't typically go with top-level `const` on non-pointer/reference
> > types, so I'd drop the `const`.
> I am not sure to understand the reasoning behind it. IMHO it's interesting
> when reading the code to know that this value will never change accross the
> function. Is there an issue I am missing?
It's not that it's an unreasonable coding style, it's that we don't want to
introduce new inconsistencies in the existing code. If the local code already
does it, we let it slide, but otherwise, we typically don't do it for local
variables or parameters (but will sometimes do it for member declarations
because that can lead to better codegen).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89490/new/
https://reviews.llvm.org/D89490
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
