-
Notifications
You must be signed in to change notification settings - Fork 105
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.
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
Fix warnings from GCC 13 #3387
Fix warnings from GCC 13 #3387
Conversation
6e71795
to
4f1a31a
Compare
// | | ||
// S(a) | ||
// This optimizer prunes such redundant joins. | ||
/* Due to the nature of graph pattern, a (node)-[rel]-(node) is always interpreted as two joins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, was this causing a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was giving the warning multi-line comment [-Werror=comment
specifically on the line with the backslash
From the gcc docs:
Warn whenever a comment-start sequence ‘/*’ appears in a ‘/*’ comment, or whenever a backslash-newline appears in a ‘//’ comment. This warning is enabled by -Wall.
Basically it thinks you're trying to use a single line comment as a multi-line by including a backslash+newline:
// HJ
// / \
E(e) S(b)
// |
// S(a)
Because technically the above would be valid.
#pragma GCC diagnostic push | ||
#pragma GCC diagnostic ignored "-Wdeprecated-volatile" | ||
#pragma clang diagnostic push | ||
#pragma clang diagnostic ignored "-Wdeprecated-volatile" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now GCC 13 complains that pragma clang is an unknown pragma. I'm not sure we can win this one without globally opting out.
@@ -22,7 +24,8 @@ common::timestamp_t ParquetTimeStampUtils::parquetTimestampNsToTimestamp(const i | |||
|
|||
int64_t ParquetTimeStampUtils::impalaTimestampToMicroseconds(const Int96& impalaTimestamp) { | |||
int64_t daysSinceEpoch = impalaTimestamp.value[2] - JULIAN_TO_UNIX_EPOCH_DAYS; | |||
auto nanoSeconds = *reinterpret_cast<const int64_t*>(impalaTimestamp.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acquamarin should take a look here and add a test if needed.
4f1a31a
to
7dc5cbd
Compare
(cherry picked from commit 2043c0a)
There are a bunch of warnings piling up with GCC 13, which I use personally, which don't show up in CI (with GCC 11).
Maybe this is too much for one PR, but it's mostly minor things.
There are two bugs I found when going through the warnings:
CastArrayHelper::checkCompatibleNestedTypes
was falling through. This meant it was allowingMAP
/STRUCT
to be cast toLIST
/ARRAY
.STRUCT
toLIST
/ARRAY
produces an unexpected error if you try that isn't very helpful, but it is actually possible to successfully castMAP(<T1>, <T2>)
to aSTRUCT(KEY <T1>, VALUE <T2>)[]
.ParquetTimeStampUtils::impalaTimestampToMicroseconds
was casting the impala timestampInt96
data to a pointer and dereferencing it. I'm fairly sure it's not supposed to be storing a pointer, and I don't think this code is covered by tests. Additionally, using a pointer to cast from twoint32_t
to oneint64_t
also throws an aliasing error. As far as I can tell it's being used to store raw data read from disk, and the cast should be safe as long as the endianness matches (I don't know if parquet has any endianness requirements, but generally we assume everything is native-endian), so I switched it to usememcpy
to work around the aliasing error.Other changes:
RUNTIME_CHECK
macro which unlikeKU_ASSERT
doesn't expect a boolean expression but makes for a nicer way of disabling extra code only needed for checks to avoid unused variable warnings.blob.cpp
: signedchar
is always<= 127
break
or[[fallthrough]]
.LogicalTypeID
is forward-declared withoutKUZU_API
in database.h, so there's a warning that theKUZU_API
annotations don't work if the type has been previously declared. Since it's header-only, I don't thinkKUZU_API
is actually necessary, but I'll see if it passes Windows CI first.function.h
: A subclass was using the defaultoperator=
, which produces a warning if there is a non-default copy constructor. But the explicit copy constructor is identical to the default so I just made it default.plan_printer.h
(similar elsewhere): Unnecessary checks that unsigned values are>= 0
.npy_reader.cpp
: Value may be uninitialized. Not really necessary to change, but initializing it isn't exactly going to slow anything down and it silences the warning.compression.cpp
: A parameter is used only by a runtime check. I added aKU_UNUSED
, which is just a macro for a(void)expr
cast to hide that warning in a way that clearly indicates the purpose (it should only be used in cases like this, otherwise the parameter name can be commented out or removed).list_column.cpp
: numValues is unsigned since the start and end variables are unsigned, but it makes more sense to look at the inputs to check for underflow rather than the result of the subtraction.system_config_test.cpp
:BufferManagerException
is polymorphic and shouldn't be caught by value (not that it actually has any subclasses).Thrift.h
:std::iterator
is deprecatedembedded_shell.cpp
: Reference to a temporary since keywordList is avector<const char*>
and the strings are being created on the fly.ParseTree.h
:operator==
doesn't really need to be virtual since it's never overridden. One subclass has anoperator==
with a different signature that hides the original and was producing a warning.constant_time.cpp
: gcc was complaining that"-Wdeprecated-volatile"
is unknown. Presumably because it's C code and the warning is only enabled in C++ mode (they're all cpp files, but we're setting theC_STANDARD 99
property via CMake). Update: Clang doesn't appear to have this distinction, I'll see if I can suppress it in a clang-specific way.