pulsar-edit / superstring

Native core components for Pulsar
MIT License
1 stars 5 forks source link

src: Include cstdint in regex.h (fix building on g++ 13) #7

Closed DeeDeeG closed 11 months ago

DeeDeeG commented 12 months ago

I basically don't know anything about C or C++, other than this fixes the compilation error on g++ 13.

I think there's some controversy as to whether including standard library stuff in this manner is a good idea, per StackOverflow?? Something about polluting namespaces?? https://stackoverflow.com/questions/13642827/cstdint-vs-stdint-h If someone more knowledgeable or experienced with C/C++ could weigh in, that'd be great, and I'd much appreciate it.

Tested working in an Ubuntu 23.10 Docker container. Would be nice to see this compile on macOS and Windows, too. (EDIT: Okay, it's working on all three OSes, phew. I can breathe easier about that now. Failure on Windows with Node 14 is unrelated to the current PR, I figured it out in another PR, see comment here: https://github.com/pulsar-edit/superstring/pull/6#issuecomment-1777757302. I have a commit in that PR branch with a fix.)

Credit to these folks for the solution: https://github.com/EdgeTX/edgetx/issues/3222. I just copied what they did, to be quite honest. (See the PR that shows as mentioning that issue a little down in the thread. That's where they implemented the fix I copied. But I found the issue first, then the PR.)

Before:

[ . . . ]
  CXX(target) Release/obj.target/superstring_core/src/core/regex.o
In file included from ../src/core/regex.cc:1:
../src/core/regex.h:17:27: error: 'uint32_t' has not been declared
   17 |   Regex(const char16_t *, uint32_t, std::u16string *error_message, bool ignore_case = false, bool unicode = false);
      |                           ^~~~~~~~
../src/core/regex.cc:50:1: error: no declaration matches 'Regex::Regex(const char16_t*, uint32_t, std::u16string*, bool, bool)'
   50 | Regex::Regex(const char16_t *pattern, uint32_t pattern_length, u16string *error_message, bool ignore_case, bool unicode) {
      | ^~~~~
[ . . . ]

(various errors in src/core/regex.cc about undeclared integer functionality we're trying to use...)

After:

[ . . . ]
  CXX(target) Release/obj.target/superstring_core/src/core/point.o
  CXX(target) Release/obj.target/superstring_core/src/core/range.o
  CXX(target) Release/obj.target/superstring_core/src/core/regex.o
  CXX(target) Release/obj.target/superstring_core/src/core/text.o
  CXX(target) Release/obj.target/superstring_core/src/core/text-buffer.o
[ . . . ]

(regex.o output compiles without even a compiler warning, at least with whatever compiler flags node-gyp is using.)

DeeDeeG commented 12 months ago

Once again, the NodeJS 14 on Windows failure is unrelated to this change.

I explained it here: https://github.com/pulsar-edit/superstring/pull/6#issuecomment-1777757302

Fix is here: https://github.com/pulsar-edit/superstring/pull/6/commits/0e62de36149b53d1a5211e0d098a48a53a068166

confused-Techie commented 12 months ago

Just adding me two cents, as it's been years since I looked at this language, and have never looked at this codebase. I think the fact tests are still running is promising, and if this positively fixes an issue that was broken prior to this change I'd say lets get it merged.

And normally I'd ping someone that's more equipped to review this, but I don't think that someone exists on this team. I know @mauricioszabo has worked on this repo before, but I believe they have also said this isn't their language of choice either, so hard to know if they have any big insights or not.

But essentially, if nobody comes out of the woodwork with better advice in the next few days, here's my light approval to get this one merged. Love the effort, and think it's awesome to step out of comfort zones to solve issues

mauricioszabo commented 12 months ago

Well, if this fixes the issue, let's go for it! There's no harm in adding any header in C++ - it won't grow up the binary name if it's not used or anything :)

DeeDeeG commented 11 months ago

Thanks, folks.

I'll go ahead and merge this now.

Hopefully no issues arise, but we can come back to it or revert for some alternative approach if necessary. This seems to have no obvious issues so far. (If I understood these things better I could be more confident, but for now the best I can do is try it and see there's no obvious indications of issues, such as yes CI is indeed passing.)