Skip to content

Include cstdint for GCC-13 #245

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
Merged
merged 6 commits into from
Mar 18, 2025
Merged

Include cstdint for GCC-13 #245

merged 6 commits into from
Mar 18, 2025

Conversation

toge
Copy link
Contributor

@toge toge commented Jun 26, 2024 •
edited
Loading

There are several compilation errors with gcc 13.
You need to include cstdint and add the std:: prefix as described in the error message.

This fix is within the scope of the C++ standard and should not affect other compilers.

c++ -std=c++17 -O3 -c -o src/util/sqlhelper.o src/util/sqlhelper.cpp
In file included from src/sql/CreateStatement.h:4,
                 from src/sql/CreateStatement.cpp:1:
src/sql/ColumnType.h:29:34: error: 'int64_t' has not been declared
   29 |   ColumnType(DataType data_type, int64_t length = 0, int64_t precision = 0, int64_t scale = 0);
      |                                  ^~~~~~~
src/sql/ColumnType.h:29:54: error: 'int64_t' has not been declared
   29 |   ColumnType(DataType data_type, int64_t length = 0, int64_t precision = 0, int64_t scale = 0);
      |                                                      ^~~~~~~
src/sql/ColumnType.h:29:77: error: 'int64_t' has not been declared
   29 |   ColumnType(DataType data_type, int64_t length = 0, int64_t precision = 0, int64_t scale = 0);
      |                                                                             ^~~~~~~
src/sql/ColumnType.h:31:3: error: 'int64_t' does not name a type
   31 |   int64_t length;     // Used for, e.g., VARCHAR(10)
      |   ^~~~~~~
src/sql/ColumnType.h:5:1: note: 'int64_t' is defined in header '<cstdint>'; this is probably fixable by adding '#include <cstdint>'
    4 | #include <ostream>
  +++ |+#include <cstdint>
    5 | 
src/sql/ColumnType.h:32:3: error: 'int64_t' does not name a type
   32 |   int64_t precision;  // Used for, e.g., DECIMAL (6, 4) or TIME (5)
      |   ^~~~~~~
src/sql/ColumnType.h:32:3: note: 'int64_t' is defined in header '<cstdint>'; this is probably fixable by adding '#include <cstdint>'
src/sql/ColumnType.h:33:3: error: 'int64_t' does not name a type
   33 |   int64_t scale;      // Used for DECIMAL (6, 4)
      |   ^~~~~~~
src/sql/ColumnType.h:33:3: note: 'int64_t' is defined in header '<cstdint>'; this is probably fixable by adding '#include <cstdint>'

@Bouncner
Copy link
Contributor

Thanks for the fix. My only issue with this PR is that I am not a fan of prefixing simple basic types with std::. I would work without the prefix, wouldn't it?

@Bouncner
Copy link
Contributor

@toge : can you check if changing g++/gcc13 to version 14 in the GH workflow already works? I think Ubuntu 24.04 ships GCC 14 or at least has install options for it.

Bouncner
Bouncner reviewed Jun 27, 2024
View reviewed changes
src/util/sqlhelper.cpp Outdated
@@ -15,7 +16,7 @@ std::ostream& operator<<(std::ostream& os, const DatetimeField& datetime);
std::ostream& operator<<(std::ostream& os, const FrameBound& frame_bound);

std::string indent(uintmax_t num_indent) { return std::string(num_indent, '\t'); }
void inprint(int64_t val, uintmax_t num_indent) { std::cout << indent(num_indent).c_str() << val << " " << std::endl; }
void inprint(std::int64_t val, uintmax_t num_indent) { std::cout << indent(num_indent).c_str() << val << " " << std::endl; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an example why I don't like the prefix. We would also need to add it to many other things (here uintmax_t) to make it consistent, right?

@toge
Copy link
Contributor Author

toge commented Jul 7, 2024 •
edited
Loading

@Bouncner
Sorry for not responding to your reply.
I haven't had a lot of time recently, but I will try to respond to the points you pointed out within the next couple of weeks.

@toge
Copy link
Contributor Author

toge commented Jul 21, 2024

@Bouncner
Sorry for the delay.
I changed this PR to include stdint.h instead of cstdint to avoid compilation errors with gcc13 and later without std:: prefix.

This modification will allow compiling with gcc13 without using std:: and without affecting the previous gcc.

Could you review it again?

Can you check if changing g++/gcc13 to version 14 in the GH workflow already works? I think Ubuntu 24.04 ships GCC 14 or at least has install options for it.

I checked sql-parser works fine on gcc14 without this PR on Fedora Linux 40.
This PR seems to be only for gcc 13 now.

@toge toge changed the title Please include cstdint for gcc13 or later Please include cstdint for gcc13 Jul 21, 2024
dey4ss
dey4ss reviewed Jul 22, 2024
View reviewed changes
src/sql/Expr.cpp Outdated
@@ -1,6 +1,7 @@
#include "Expr.h"
#include <stdio.h>
#include <string.h>
#include <stdint.h>
Copy link
Member

@dey4ss dey4ss Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Pick here: Please have a look at the include order. Running the format.sh script should already fix this.

I also started a workflow run. Not sure what happened to the gcc-6 stage though. However, we already have a gcc-13 runner there since #242.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might have a problem with Ubuntu 18. But where does node come from? It's from the GitHub action checkout? Curious how that can fail.

https://stackoverflow.com/a/74912791

Copy link
Member

Choose a reason for hiding this comment

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

Fixed with #246
@toge to pass the CI, you need to merge the current master.

@dey4ss dey4ss mentioned this pull request Jul 25, 2024
Merged
@Bouncner
Copy link
Contributor

Hi @toge, I would go ahead and merge the main branch, but since you are working on a fork, I am not able to.

Are you still interested in getting this PR landed? If you don't have the time, I would take it from here.

@toge
Copy link
Contributor Author

toge commented Mar 18, 2025

@Bouncner
Sorry for suspending this PR.

Please go a head.
I am happy if this library works on much more environment.

@Bouncner
Copy link
Contributor

Hi @toge, I would go ahead and merge the main branch, but since you are working on a fork, I am not able to.

Are you still interested in getting this PR landed? If you don't have the time, I would take it from here.

Oh ... did not know I can resolve conflicts on my own here.

Bouncner
Bouncner approved these changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@Bouncner Bouncner left a comment

Choose a reason for hiding this comment

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

LGTM.

@Bouncner Bouncner changed the title Please include cstdint for gcc13 Include cstdint for GCC-13 Mar 18, 2025
@Bouncner Bouncner merged commit 139375e into hyrise:main Mar 18, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dey4ss dey4ss dey4ss left review comments

@Bouncner Bouncner Bouncner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@toge @Bouncner @dey4ss