aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8849
break;
-
// HLSL attributes:
----------------
hctim wrote:
> aaron.ballman wrote:
> > Spurious whitespace change?
> unfortunately there's no way in my editor to trim trailing whitespace only on
> changed lines :(, so i end up fixing things like this drive-by.
>
> let me know if you feel very strongly about this diff and I can kill it, but
> I personally think the drive-by-fix isn't a huge problem and the alternative
> of whitespace-fix-only commit seems a bit overkill
Personally, I don't feel very strongly because the chances of the whitespace
being someone's entrypoint to git-blame is pretty minimal (especially given
there's only one change here). However, we typically still ask for formatting
changes to be separated out
(https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access #2) rather
than lumped in with functional changes so reviewers will ask for these sort of
changes to be backed out, so this may crop up repeatedly if your editor doesn't
give you the options you need.
================
Comment at: clang/test/CodeGen/hwasan-globals.cpp:1-2
+// RUN: echo "int extra_global;" > %t.extra-source.cpp
+// RUN: echo "global:*ignorelisted_global*" > %t.ignorelist
+// RUN: %clang_cc1 -include %t.extra-source.cpp -fsanitize=hwaddress
-fsanitize-ignorelist=%t.ignorelist -emit-llvm -o - %s | FileCheck %s
--check-prefixes=CHECK
----------------
hctim wrote:
> aaron.ballman wrote:
> > Are these files automatically deleted when the test is done because we're
> > using %t, or do we need to clean those up manually?
> AFAIK nothing is ever automatically deleted (e.g. the outputs of the
> compiler). Is automated cleanup here necessary?
Necessary? Probably not (I'd expect these to go into the temp directory). A
kindness so disks don't fill up? Probably. Because the content here is static
anyway, these could just be files in the `Inputs` directory so they don't need
to be created every time the test is run.
================
Comment at: compiler-rt/test/hwasan/TestCases/global-with-reduction.c:25
-int x = 1;
+#include <stdlib.h>
----------------
hctim wrote:
> aaron.ballman wrote:
> > I'm not a compiler-rt expert, but is this valid? I assume this is using the
> > system stdlib.h which is not something we usually want in lit tests.
> >
> > I think that's why `atoi` was previously being forward declared; then we
> > don't need to include the whole header file.
> I don't see there being any problem with including stdlib.h here, it's done
> in lots of other compiler-rt tests.
>
> I patched this up because the compiler actually complains about
> forward-declaring c library functions (it's just silenced by llvm-lit by
> default).
Ah, good point about other tests doing this (it's a restriction we have in
Clang tests, but not here from what I can tell).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127544/new/
https://reviews.llvm.org/D127544
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
