george.burgess.iv updated this revision to Diff 140675.
george.burgess.iv marked 5 inline comments as done.
george.burgess.iv added a comment.
Addressed feedback
https://reviews.llvm.org/D45059
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
george.burgess.iv added a comment.
> C89 has no bool type and no stdbool.h but it has been added to later
> standards? That means the generalization could theoretically be done for
> later C standards, because we could check if the bool typedef had been used?
> If yes, would the check benefit?
george.burgess.iv added a comment.
Thanks for the feedback!
> You noted, that the C++ warning would catch this case, but does not get
> triggered in C. Would it be wort to generalize the concern and have a warning
> that catch the comparison, regardless of the macro?
I'm not quite sure how we
george.burgess.iv updated this revision to Diff 140459.
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Addressed feedback
https://reviews.llvm.org/D45059
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
george.burgess.iv added a comment.
Thanks!
> Will be good idea to clarify where TEMP_FAILURE_RETRY come from.
Updated the CL summary and added "TEMP_FAILURE_RETRY is a macro provided by
both glibc and Bionic." to ComparisonInTempFailureRetryCheck.h. Did you have
anything else in mind?
george.burgess.iv updated this revision to Diff 140324.
george.burgess.iv marked 3 inline comments as done.
george.burgess.iv edited the summary of this revision.
george.burgess.iv added a comment.
Addressed feedback.
https://reviews.llvm.org/D45059
Files:
george.burgess.iv created this revision.
george.burgess.iv added reviewers: aaron.ballman, alexfh.
Herald added a subscriber: mgorny.
`TEMP_FAILURE_RETRY(x)` is a macro that executes `x` until `(x) != -1 || errno
!= EINTR`, and evaluates to the result of the last execution of `x`.
I've been
george.burgess.iv added inline comments.
Comment at: test/Sema/builtin-object-size.c:105
+void rd36094951_IAS_builtin_object_size_assertion(IncompleteArrayStruct* p) {
+ __builtin___strlcpy_chk (p->session[0].string, "ab", 2,
__builtin_object_size(p->session[0].string, 1));
george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.
LGTM assuming my nit gets addressed.
Thank you!
> Maybe someone who's more familiar with this builtin could point to the cause
> of this discrepancy
Yeah, the
george.burgess.iv added a comment.
Thanks for working on this!
Unfortunately, I've never touched demanglers before, so I don't think I can
LGTM this patch.
> This attribute is mangled as if it was meant to match the
> production.
Yup, definitely a mistake. Apologies. :)
george.burgess.iv added a comment.
> That said, one of the upsides of the current ubsan is that whether it will
> produce a diagnostic is predictable (as long as you don't use uninitialized
> data); you lose that to some extent with llvm.objectsize because it depends
> on the optimizer.
True.
george.burgess.iv added a comment.
If the answer to my question is "no, it'll just work," LGTM. Thanks!
Comment at: test/ubsan/TestCases/Misc/bounds.cpp:9
+int get_int(int *const p __attribute__((pass_object_size(0))), int i) {
+ // CHECK-A-2: bounds.cpp:[[@LINE+1]]:10:
george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks again!
https://reviews.llvm.org/D40940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv added a comment.
Thanks for this!
It's interesting to me that these array-bound checks don't seem to use
`@llvm.objectsize` in some form already. I can't find any notes about it
grepping through git/source, so I'm happy with it.
Comment at:
george.burgess.iv updated this revision to Diff 119499.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Addressed feedback. Thanks!
After looking around and internalizing a little bit of how backends in LLVM
work, the path forward I have in mind is to
george.burgess.iv added a comment.
> However, the tests cover floating point, but they don't cover vector calls
> (arm_neon.h).
`#include ` gives me ~12,000 errors, presumably because there are
so many functions that take vectors/floats defined in it. The hope was that
calling `bar` and `foo`
george.burgess.iv added a comment.
I like the idea of fixing those things, too; I'll start poking them soon. :)
Even if we do end up fixing all of that, I still think it would be good to try
to diagnose this in the frontend. So, if anyone has comments on this while I'm
staring at the aarch64
george.burgess.iv created this revision.
Herald added a subscriber: javed.absar.
(Copy/pasting the reviewer list from https://reviews.llvm.org/D26856.)
Addresses https://bugs.llvm.org/show_bug.cgi?id=30792 .
In GCC, -mgeneral-regs-only emits errors upon trying to emit floating-point or
vector
george.burgess.iv added a comment.
Thanks for working on this!
One small drive-by nit for you. Same "no need to update the diff this very
second" as vsk; I can't LGTM this with confidence.
(Also, in the future, please try to include context
george.burgess.iv abandoned this revision.
george.burgess.iv added a comment.
r306467 used `__has_extension(overloadable_unmarked)` instead.
Thanks again for the comments!
https://reviews.llvm.org/D33904
___
cfe-commits mailing list
This revision was automatically updated to reflect the committed changes.
Closed by commit rL306467: [Sema] Allow unmarked overloadable functions.
(authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D32332?vs=103448=104277#toc
Repository:
rL LLVM
george.burgess.iv added a comment.
Woohoo! Thanks again to both of you.
https://reviews.llvm.org/D32332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv added a comment.
Thank you both for the comments!
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3294-3295
+def err_attribute_overloadable_multiple_unmarked_overloads : Error<
+ "at most one 'overloadable' function for a given name may lack the "
+
george.burgess.iv updated this revision to Diff 103448.
george.burgess.iv marked 6 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback
https://reviews.llvm.org/D32332
Files:
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
george.burgess.iv updated this revision to Diff 102850.
george.burgess.iv marked 3 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback
https://reviews.llvm.org/D32332
Files:
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
george.burgess.iv updated this revision to Diff 102207.
george.burgess.iv added a comment.
Swap to using `__has_feature(overloadable_unmarked)` for detection, as
recommended by Hal in https://reviews.llvm.org/D33904
https://reviews.llvm.org/D32332
Files:
include/clang/Basic/AttrDocs.td
george.burgess.iv added a comment.
> Why not just use __has_feature(overloadable_unmarked) or similar?
My impression was that `__has_feature` was was for larger features than tweaks
to attributes. If this would be an appropriate use of `__has_feature`, though,
I'm happy to keep things simple.
george.burgess.iv added a comment.
Ping :)
https://reviews.llvm.org/D33904 is what I imagine would be a good way to detect
this change.
https://reviews.llvm.org/D32332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv created this revision.
Herald added a subscriber: javed.absar.
Attributes may gain features or added flexibility over time. This patch aims to
add a simple and uniform way to directly add/query for arbitrary changes in
attributes, instead of having to rely on other information
george.burgess.iv updated this revision to Diff 100201.
george.burgess.iv added a comment.
Herald added a subscriber: jfb.
Fix the aforementioned issue; PTAL.
Note that this fix is slightly backwards incompatible. It disallows code like:
void foo(int);
void foo(int)
george.burgess.iv added a comment.
I'd be happy with that approach. Do you like it, Aaron?
FWIW, I did a bit of archaeology, and it looks like the commit that added the
requirement that all overloads must have `overloadable` (r64414) did so to keep
users from "trying to be too sneaky for their
george.burgess.iv added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:2921
+ const auto *NewOvl = New->getAttr();
+ if (NewOvl->isTransparent() != OldOvl->isTransparent()) {
+assert(!NewOvl->isImplicit() &&
aaron.ballman wrote:
> Can
george.burgess.iv updated this revision to Diff 98569.
george.burgess.iv marked 7 inline comments as done.
george.burgess.iv added a comment.
- Addressed all feedback
- Now we emit a warning when `transparently_overloadable` "overrides"
`overloadable`, rather than an error
george.burgess.iv updated this revision to Diff 97698.
george.burgess.iv added a comment.
We now require `transparently_overloadable` on all redeclarations. Allowing
mixed `transparently_overloadable` and `overloadable` coming soon.
https://reviews.llvm.org/D32332
Files:
george.burgess.iv added a comment.
> I'd like to understand this use case a little bit better. If you don't
> perform the mangling, then choosing which overload gets called is already
> based on the source order, isn't it? See https://godbolt.org/g/8b10Ns
I think I might have miscommunicated:
george.burgess.iv added a comment.
thanks for the feedback!
fwiw, at a high level, the main problem i'm trying to solve with this is that
you can't really make a `__overloadable_without_mangling` macro without handing
it the function name, since you currently need to use
george.burgess.iv added a comment.
ping :)
https://reviews.llvm.org/D32332
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv created this revision.
One of the common use-cases for the `overloadable` attribute is to create small
wrapper functions around an existing function that was previously not
overloadable. For example:
// C Library v1
void foo(int i);
// C Library v2
// Note the
george.burgess.iv added inline comments.
Comment at: test/Driver/debug-options.c:201
//
+// GRECORD: "-dwarf-debug-flags"
+// GRECORD: -### -c -grecord-gcc-switches
echristo wrote:
> This seems a little light on the testing, would you mind adding some more
>
george.burgess.iv added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:4328
+ if (getToolChain().UseDwarfDebugFlags() ||
+ Args.hasArg(options::OPT_grecord_gcc_switches)) {
ArgStringList OriginalArgs;
looks like we have to consider
george.burgess.iv added a comment.
This is looking pretty good! Aside from a few nits, I'm happy with this after
we add tests.
Comment at: lib/Driver/ToolChains/Clang.cpp:2725
+ if (Args.hasArg(options::OPT_grecord_gcc_switches)) {
+std::string CmdLineOpts = "";
+for
This revision was automatically updated to reflect the committed changes.
Closed by commit rL294800: Don't let EvaluationModes dictate whether an invalid
base is OK (authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D29469?vs=86870=88061#toc
Repository:
rL LLVM
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Thanks!
https://reviews.llvm.org/D29469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv added a comment.
End of week ping :)
https://reviews.llvm.org/D29469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL294662: Add support for armv7ve flag in clang (PR31358).
(authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D29773?vs=87831=87899#toc
Repository:
rL LLVM
george.burgess.iv created this revision.
We're currently using a special EvaluationMode to determine whether
we're OK with invalid base expressions during objectsize evaluation.
Using it to figure out how we handle UB/etc. is fine, but I think it's
too far-reaching to use for checking whether
george.burgess.iv added a comment.
Thanks for the review!
Repository:
rL LLVM
https://reviews.llvm.org/D28889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293360: Change how we handle diagnose_if attributes.
(authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D28889?vs=86146=86155#toc
Repository:
rL LLVM
george.burgess.iv added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:11933
}
-
aaron.ballman wrote:
> Unintended change?
...I dunno what keeps making this change, but I'll re-undo it before I submit.
https://reviews.llvm.org/D28889
george.burgess.iv added inline comments.
Comment at: test/SemaCXX/diagnose_if.cpp:615
+// evaluator isn't able to evaluate `adl::Foo(1)` to a constexpr, though.
+// I'm assuming this is because we assign it to a temporary.
+for (void *p : adl::Foo(1)) {}
george.burgess.iv updated this revision to Diff 86146.
george.burgess.iv marked 7 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback
> Another "fun" testcase
Ooh, shiny. Added.
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
george.burgess.iv updated this revision to Diff 86111.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Address feedback
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
george.burgess.iv added a comment.
Thanks for the feedback!
Comment at: lib/Sema/SemaChecking.cpp:2520
+
+// TODO: Call can technically be a const CallExpr, but const_casting feels
ugly,
+// and I really don't want to duplicate unwrapCallExpr's logic. No caller
really
george.burgess.iv added a comment.
Pinging early because this is a release blocker. :)
https://reviews.llvm.org/D28889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv updated this revision to Diff 85486.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback.
Richard noted that, because we're now doing these checks after overload
resolution has occurred, we no longer need to convert
george.burgess.iv updated this revision to Diff 84988.
george.burgess.iv added a comment.
Sprinkle in a few `const`s, use `const auto *` in range for loops.
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
george.burgess.iv created this revision.
As it turns out, emitting diagnostics from places where you're not meant to
emit them from is a very bad idea. :)
After some looking around, it seems that it's less insane to check for
`diagnose_if` attributes in code that's already checking for e.g.
george.burgess.iv added a comment.
Thanks for the review! :)
Repository:
rL LLVM
https://reviews.llvm.org/D27424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL291418: Add the diagnose_if attribute to clang. (authored by
gbiv).
Changed prior to commit:
https://reviews.llvm.org/D27424?vs=83412=83584#toc
Repository:
rL LLVM
https://reviews.llvm.org/D27424
george.burgess.iv added a comment.
> We do:
> http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
Awesome. Thanks!
> Btw, there's a question in there about late parsing that Phab won't let me
> delete, feel free to ignore it. ;-)
OK. The answer is
george.burgess.iv updated this revision to Diff 83412.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Addressed feedback. Thanks!
https://reviews.llvm.org/D27424
Files:
include/clang/AST/Expr.h
include/clang/Basic/Attr.td
george.burgess.iv added a comment.
Do we have a page that details when we should/shouldn't use `auto`? I was under
the impression that it was preferred only in cases where the type's spelled out
(e.g. `cast`, ...). (To be clear, I'm happy to use it in loops, too; I'd
just like to know if we
george.burgess.iv updated this revision to Diff 83188.
george.burgess.iv marked 18 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback + made `diagnose_if` late-parsed.
https://reviews.llvm.org/D27424
Files:
include/clang/AST/Expr.h
george.burgess.iv abandoned this revision.
george.burgess.iv added a comment.
Committed in https://reviews.llvm.org/rL290149. Thanks again!
https://reviews.llvm.org/D26410
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
This revision was automatically updated to reflect the committed changes.
Closed by commit rL290149: Add the alloc_size attribute to clang. (authored by
gbiv).
Changed prior to commit:
https://reviews.llvm.org/D14274?vs=77222=82046#toc
Repository:
rL LLVM
https://reviews.llvm.org/D14274
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Thanks, everyone! :)
Repository:
rL LLVM
https://reviews.llvm.org/D14274
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.burgess.iv updated this revision to Diff 81259.
george.burgess.iv marked an inline comment as done.
george.burgess.iv added a comment.
Addressed alignment comment. Thanks! (Late parsing is still to-be-done)
https://reviews.llvm.org/D27424
Files:
include/clang/Basic/Attr.td
george.burgess.iv added a comment.
Glad you like it!
> The second thing that would be amazing if this attribute was late parsed so
> it the ability to reference this when applied to member functions
That seems like a good idea; I'll look into what that would take. (In the
meantime, reviews
george.burgess.iv added a comment.
Ping :)
https://reviews.llvm.org/D14274
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.burgess.iv created this revision.
george.burgess.iv added reviewers: rsmith, aaron.ballman.
george.burgess.iv added subscribers: EricWF, cfe-commits.
This patch adds the `diagnose_if` attribute to clang.
`diagnose_if` is meant to be a less powerful version of `enable_if`: it doesn't
george.burgess.iv added a comment.
Ping
https://reviews.llvm.org/D14274
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
101 - 171 of 171 matches
Mail list logo
