Support compiler-agnostic annotations #2456
Conversation
erimatnor
force-pushed
the
compiler-agnostic-annotations
branch
4 times, most recently
from
824b44d
to
bd8a003
Compare
|
Recently updated my Mac, which now has LLVM/Clang 12.0, which could no longer compile TimescaleDB since it seems to no longer accept |
erimatnor
force-pushed
the
compiler-agnostic-annotations
branch
from
bd8a003
to
4e91736
Compare
Codecov Report
@@ Coverage Diff @@
## master #2456 +/- ##
==========================================
+ Coverage 90.10% 90.12% +0.01%
==========================================
Files 213 213
Lines 34336 34356 +20
==========================================
+ Hits 30940 30964 +24
+ Misses 3396 3392 -4
Continue to review full report at Codecov.
|
src/annotations.h
Outdated
| #else | ||
| #error "unsupported compiler" | ||
| #endif |
There was a problem hiding this comment.
I don't think you should generate an error if there is no way to annotate a fall-through. Either the compile will complain about that, or not, but there is no reason to prevent it from compiling.
| /* Fall-through annotation */ | ||
| #if defined(__clang__) | ||
| #if (__clang_major__ >= 12) || (__clang_analyzer__) | ||
| #define TS_FALLTHROUGH __attribute__((fallthrough)) |
There was a problem hiding this comment.
Just to note that it is also possible to write /* fallthough */ comment which will be recognized, maybe it could be easier to support in some sense
There was a problem hiding this comment.
This was the issue. It seems to no longer be supported in clang 12 (at least on the mac)
This change adds a compiler-agnostic annotation for fall-throughs in switch statements. The definition is put in `annotations.h`, which can be expanded to hold definitions for similar functionality in the future. The `clang` compiler (as of version 12) seems to have have dropped support for the previous comment-based annotations to allow fall-throughs in favor of native annotations or GCC-style attributes.
erimatnor
force-pushed
the
compiler-agnostic-annotations
branch
from
4e91736
to
fb7fa7f
Compare
| #if (__clang_major__ >= 12) || (__clang_analyzer__) | ||
| #define TS_FALLTHROUGH __attribute__((fallthrough)) | ||
| #else | ||
| #define TS_FALLTHROUGH /* FALLTHROUGH */ |
There was a problem hiding this comment.
One thing that worries me here is that both comments and macros symbols are handled by the preprocessor. Have you tested this with a compiler that uses the /* FALLTHROUGH */ syntax? I think that the standard builds-on-push do that but can you please verify before pushing.
There was a problem hiding this comment.
We used the /* FALLTHROUGH */ syntax prior to this PR. But clang seems to have dropped support for it in favor of the GCC-compatible attribute. AFAIK, the CI still runs a pre 12 clang, so should still use /* FALLTHROUGH */.
This change adds a compiler-agnostic annotation for fall-throughs in
switch statements. The definition is put in
annotations.h, which canbe expanded to hold definitions for similar functionality in the
future.
The
clangcompiler (as of version 12) seems to have have droppedsupport for the previous comment-based annotations to allow
fall-throughs in favor of native annotations or GCC-style attributes.