[FIXED] C++ compiler errors #832

mtmk · 2025-01-14T22:14:14Z

Fixes the case where C++ compiler errors with C2079 on C style struct definitions. Also added a simple cpp file to make sure future changes won't cause the same issue.

Error happens when natsp.h is included in a .cpp file:

D:\src\nats.c\src\natsp.h(377): error C2079: '__jsFetch::opts' uses undefined struct 'jsOptionsPullSubscribeAsync'

Fixes the case where C++ compiler errors with C2079 on C style struct definitions. Also added a simple cpp file to make sure future changes won't cause the same issue.

kozlovic

There are lot of build failures in GA (some may be unrelated and a temporary issue). But regardless, I think that the changes are not necessary since no application (cpp or c) should include natsp.h directly.

kozlovic · 2025-01-14T22:20:51Z

examples/check_cpp.cpp

@@ -0,0 +1,3 @@
+// Check if the C++ compiler is working
+// against C style struct definitions.
+#include "natsp.h"


This file is private: the p is for private. No application should include this file directly. Actually, it should not even be available if the make install since they would have access only to the nats.h file (not the private version).

So I would remove this file (which by the way does not compile - see Windows GA failure) and undo the change in examples/CMakeLists.txt.

no you're right when the library is installed natsp.h isn't available. I guess it's only affecting the source copy builds.

Do you think having a some form of cpp file for at least making sure C++ compiler will be happy in these kinds of scenarios is a good thing? if so how would you go about adding a cpp file to the build?

No, because again, we should not have an app include nastp.h, so there is no need to have that "work". See the other comment I just posted regarding the extraction of the js option.

kozlovic · 2025-01-14T22:22:35Z

src/nats.h

+ *
+ * Part of #jsOptions.
+ */
+typedef struct jsOptionsPullSubscribeAsync


I have no issue making the jsOptionsPullSubscribeAsync a typedef structure, instead of being "embedded" in jsOpts, but if the issue is only because a user tries to include natsp.h, then I would not consider changing anything.

yes I think that is the case. do you think moving to typedefs is still worth while for future proofing?

At this point no (seeing all the build failures).

At the very least, what you could do is update the PR by removing the cpp file and changes to examples' make file. Keep the changes in nats.h. Assuming there is no build issue, this PR should still be updated because then we would need to require a new version here because the header is changed.

If there is still build failures, have a new branch that is just pointing to main and push to see if CI still has build issues. If not, then let's close this PR. If there is still build issues, they may be temporary and I would feel better wait for those issues to be resolved with the main branch before trying to include this modified PR.

We got into the current state because we include natsp.h because we wrote our own watcher around the kvWatcher. We did that so we could invoke a callback on the data. This is all before I got here. I will be looking into the NATS API around this private data structure as soon as I post this, so I haven't yet looked at the API as it exists. Is there an API that would invoke a callback per message that we either missed, or you added after this implementation?

ok removed the cpp and reverted cmake. build seems fine now apart from couple of flappers presumably.

edit: should I move rest of the nested definitions for consistency?

@mtmk Then we should still update the main make file as I mentioned earlier. That being said, it could be done as part of the next release. Also, we need to make sure that this PR does not end-up in the patch release. @levb can confirm.

Also, @PhilBarila does that mean that you are using other internals? That would not be too good, but regardless, if the change in this PR is enough to "fix" your issue, then fine.

In that case, @mtmk, the check.cpp may instead be in the test directory. It could be a subdirectory, similar to this. It would also need to be added to the tests to run like we do it here.

If this is confusing, I could do that as a separate PR later. Let me know.

The kvWatcher is the only thing we used from the natsp.h. If you have an alternative to writing our own jet-stream watcher, we'll take a very serious look at it ...

Is there an API that would invoke a callback per message that we either missed, or you added after this implementation?

@PhilBarila, you can create a watcher, but they are sync by nature, so you need to call their "next" API. I am not sure what you did exactly using the internal structure, so it is difficult for me to say if there is equivalent or not (if it is just to be a callback instead of a sync call, it sounds like a thread calling kvWatcher_Next() and invoking your callback with the result may be enough)

And pull the data out of it with kvEntry_Value ... I'll have to do more digging, but that might work. I am only just wading in, so there's lots more to learn before we have a clean path forward to stop depending on your private data ... But we'll get it done as soon as is practical with all the other competing priorities ...

codecov · 2025-01-14T22:49:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.47%. Comparing base (1553d4a) to head (47b10bb).
Report is 34 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #832      +/-   ##
==========================================
+ Coverage   68.71%   70.47%   +1.76%     
==========================================
  Files          39       47       +8     
  Lines       15207    15367     +160     
  Branches     3143     3149       +6     
==========================================
+ Hits        10449    10830     +381     
+ Misses       1700     1501     -199     
+ Partials     3058     3036      -22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

PhilBarila · 2025-01-15T00:33:47Z

As the one who brought this up, I appreciate your prompt attention. It looks just like what I would have offered. Thank you very kindly.

kozlovic

It fails to compile on Travis for some build runs, so I wonder if that would work if we statically link, you could give it a try.

kozlovic · 2025-01-15T00:44:48Z

test/check_cpp/check_cpp.cpp

@@ -0,0 +1,29 @@
+// Copyright 2017-2018 The NATS Authors


Change date to 2025.

kozlovic · 2025-01-15T00:47:12Z

test/check_cpp/CMakeLists.txt

+if(NOT BUILD_TESTING)
+  return()
+endif()
+


There are failures on Travis for clang with sanitize. Not exactly sure why. So maybe try to build statically, so add this to this make file.

kozlovic · 2025-01-15T00:47:50Z

test/check_cpp/CMakeLists.txt

+# Build the test program
+add_executable(check_cpp check_cpp.cpp)
+
+target_link_libraries(check_cpp nats ${NATS_EXTRA_LIB})


Replace nats with nats_static.

👍 that worked - current failures seem to be flappers although both travis builds failed on ReconnectFailsPendingRequest

kozlovic · 2025-01-15T00:48:40Z

src/nats.h

-                } Info;                                 ///< Optional stream information retrieval options.
-
-        } Stream;                                       ///< Optional stream options.
+        jsOptionsPublishAsync PublishAsync;                     ///< extra options for #js_PublishAsync


nit: If you could align PublishAsync;, PullSubscribeAsync; and Stream fields, that would be nice.

kozlovic

LGTM

levb

LGTM, ack that this is not for the patch release

[FIXED] C++ compiler errors

ac42801

Fixes the case where C++ compiler errors with C2079 on C style struct definitions. Also added a simple cpp file to make sure future changes won't cause the same issue.

mtmk requested review from levb and kozlovic January 14, 2025 22:14

kozlovic requested changes Jan 14, 2025

View reviewed changes

Fixing build removed check_cpp.cpp

f1f3b9d

mtmk added 3 commits January 14, 2025 23:43

Added check_cpp build

1a2b235

Moved rest of the inner structs

4c8f3d7

Fixed Windows build

f21fd51

kozlovic requested changes Jan 15, 2025

View reviewed changes

Fixing build and format

47b10bb

kozlovic approved these changes Jan 15, 2025

View reviewed changes

levb approved these changes Jan 15, 2025

View reviewed changes

levb merged commit d814c7a into main Jan 15, 2025
30 checks passed

levb deleted the mtmk-struct-cpp-fix branch January 15, 2025 14:34

github-actions bot pushed a commit that referenced this pull request Jan 15, 2025

[skip ci] Update docs: d814c7a: [FIXED] C++ compiler errors (#832)

81ee601

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED] C++ compiler errors #832

[FIXED] C++ compiler errors #832

mtmk commented Jan 14, 2025

kozlovic left a comment

kozlovic Jan 14, 2025

mtmk Jan 14, 2025

kozlovic Jan 14, 2025

kozlovic Jan 14, 2025

mtmk Jan 14, 2025

kozlovic Jan 14, 2025

PhilBarila Jan 14, 2025

mtmk Jan 14, 2025 •

edited

Loading

kozlovic Jan 14, 2025

PhilBarila Jan 14, 2025

kozlovic Jan 14, 2025

PhilBarila Jan 14, 2025

codecov bot commented Jan 14, 2025 •

edited

Loading

PhilBarila commented Jan 15, 2025 •

edited

Loading

kozlovic left a comment

kozlovic Jan 15, 2025

kozlovic Jan 15, 2025

kozlovic Jan 15, 2025

mtmk Jan 15, 2025

kozlovic Jan 15, 2025

kozlovic left a comment

levb left a comment

[FIXED] C++ compiler errors #832

[FIXED] C++ compiler errors #832

Conversation

mtmk commented Jan 14, 2025

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtmk Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

PhilBarila commented Jan 15, 2025 • edited Loading

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic left a comment

Choose a reason for hiding this comment

levb left a comment

Choose a reason for hiding this comment

mtmk Jan 14, 2025 •

edited

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading

PhilBarila commented Jan 15, 2025 •

edited

Loading