Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support compiler-agnostic annotations #2456

Merged
merged 1 commit into from Sep 29, 2020
Merged

Support compiler-agnostic annotations #2456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/annotations.h
View file
Open in desktop
@@ -0,0 +1,24 @@
/*
* This file and its contents are licensed under the Apache License 2.0.
* Please see the included NOTICE for copyright information and
* LICENSE-APACHE for a copy of the license.
*/
#ifndef TIMESCALEDB_ANNOTATIONS_H
#define TIMESCALEDB_ANNOTATIONS_H

/* Fall-through annotation */
#if defined(__clang__)
#if (__clang_major__ >= 12) || (__clang_analyzer__)
#define TS_FALLTHROUGH __attribute__((fallthrough))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the issue. It seems to no longer be supported in clang 12 (at least on the mac)

#else
#define TS_FALLTHROUGH /* FALLTHROUGH */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@erimatnor erimatnor Sep 28, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 */.

#endif /* __clang__ */
#elif defined(__GNUC__)
/* Supported since GCC 7 */
#define TS_FALLTHROUGH __attribute__((fallthrough))
#else
/* MSVC and other compilers */
#define TS_FALLTHROUGH /* FALLTHROUGH */
#endif

#endif /* TIMESCALEDB_ANNOTATIONS_H */
3 changes: 3 additions & 0 deletions src/cache_invalidate.c
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@
#include <miscadmin.h>
#include <utils/syscache.h>

#include "annotations.h"
#include "catalog.h"
#include "compat.h"
#include "extension.h"
Expand Down Expand Up @@ -130,6 +131,7 @@ cache_invalidate_xact_end(XactEvent event, void *arg)
* backends cannot have the invalid state.
*/
cache_invalidate_relcache_all();
break;
default:
break;
}
Expand All @@ -148,6 +150,7 @@ cache_invalidate_subxact_end(SubXactEvent event, SubTransactionId mySubid,
* in cache_invalidate_xact_end.
*/
cache_invalidate_relcache_all();
break;
default:
break;
}
Expand Down
3 changes: 2 additions & 1 deletion src/indexing.c
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@
#include <utils/lsyscache.h>
#include <utils/syscache.h>

#include "annotations.h"
#include "dimension.h"
#include "errors.h"
#include "hypertable_cache.h"
Expand Down Expand Up @@ -61,7 +62,7 @@ index_has_attribute(List *indexelems, const char *attrname)
break;
}
}
/* FALLTHROUGH */
TS_FALLTHROUGH;
default:
elog(ERROR, "unsupported index list element");
}
Expand Down
3 changes: 2 additions & 1 deletion src/planner.c
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@

#include <math.h>

#include "annotations.h"
#include "cross_module_fn.h"
#include "license_guc.h"
#include "hypertable_cache.h"
Expand Down Expand Up @@ -791,7 +792,7 @@ timescaledb_set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, Index rti, Rang
ts_cm_functions->set_rel_pathlist_dml(root, rel, rti, rte, ht);
break;
}
/* Fall through */
TS_FALLTHROUGH;
default:
apply_optimizations(root, reltype, rel, rte, ht);
break;
Expand Down
3 changes: 3 additions & 0 deletions src/process_utility.c
View file
Open in desktop
Expand Up @@ -38,6 +38,7 @@

#include <miscadmin.h>

#include "annotations.h"
#include "export.h"
#include "process_utility.h"
#include "catalog.h"
Expand Down Expand Up @@ -3847,6 +3848,7 @@ process_utility_xact_abort(XactEvent event, void *arg)
* transactions.
*/
expect_chunk_modification = false;
break;
default:
break;
}
Expand All @@ -3861,6 +3863,7 @@ process_utility_subxact_abort(SubXactEvent event, SubTransactionId mySubid,
case SUBXACT_EVENT_ABORT_SUB:
/* see note in process_utility_xact_abort */
expect_chunk_modification = false;
break;
default:
break;
}
Expand Down
3 changes: 2 additions & 1 deletion test/src/bgw/timer_mock.c
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@
#include <utils/tqual.h>
#endif

#include "annotations.h"
#include "timer_mock.h"
#include "log.h"
#include "scanner.h"
Expand Down Expand Up @@ -60,7 +61,7 @@ mock_wait(TimestampTz until)
WaitForBackgroundWorkerShutdown(bgw_handle);
bgw_handle = NULL;
}
/* FALLTHROUGH */
TS_FALLTHROUGH;
case IMMEDIATELY_SET_UNTIL:
ts_params_set_time(until, false);
return true;
Expand Down
3 changes: 2 additions & 1 deletion test/src/test_with_clause_parser.c
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@
#include "export.h"
#include "test_utils.h"
#include "with_clause_parser.h"
#include "annotations.h"

static DefElem *
def_elem_from_texts(Datum *texts, int nelems)
Expand All @@ -30,7 +31,7 @@ def_elem_from_texts(Datum *texts, int nelems)
break;
case 3:
elem->arg = (Node *) makeString(text_to_cstring(DatumGetTextP(texts[2])));
/* FALLTHROUGH */
TS_FALLTHROUGH;
case 2:
elem->defname = text_to_cstring(DatumGetTextP(texts[1]));
elem->defnamespace = text_to_cstring(DatumGetTextP(texts[0]));
Expand Down
4 changes: 2 additions & 2 deletions tsl/src/remote/dist_ddl.c
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@
#include <catalog/namespace.h>
#include <nodes/parsenodes.h>

#include <annotations.h>
#include <guc.h>
#include "hypertable_data_node.h"
#include "chunk_index.h"
Expand Down Expand Up @@ -421,8 +422,7 @@ dist_ddl_preprocess(ProcessUtilityArgs *args)
/* Those commands are also targets for execute_on_start in since they
* are not supported by event triggers. */
set_dist_exec_type(DIST_DDL_EXEC_ON_START);

/* fall through */
TS_FALLTHROUGH;
default:
dist_ddl_error_raise_unsupported();
break;
Expand Down