add CJSON_DECLARE macros to support gcc visibility and windows dllexport #116

mjerris · 2017-02-22T22:39:50Z

These macros allow us to activate compiler visibility flags (gcc) and dllimport/dllexport (windows).

FSMaxB · 2017-02-22T23:58:41Z

Can you please explain what this does and why it is necessary!

I was assuming that the use of static and extern for specifying the storage class was sufficient.

In general I don't want to include any macros to cJSON that check for the environment or available features and what not, as long as it is not completely unavoidable.

mjerris · 2017-02-23T01:39:23Z

sure. There are two things in here. I guess of a bit of note, on functions, extern is actually implicitly there, and you don't need to specify it (data is a bit different in this regard), your totally correct about static. There are two places this doesn't address. The first is on windows, what symbols are exposed from a dll need to be specified, either with a manual list or preferably by specifying dllexport (and when you use it dllimport) ... you can see that in the macro. The second is a similar feature added in the last 10 years or so to gcc (and clang) called visibility. This allows you to specify which symbols will be exported from an so. An example of when you might want to use visibility would be when you have a large project and you want some symbols to only be used within the so (but in different files) and others that are publicly exported. The macros I added make it possible to address all of these situations just by passing different defines at build time.

FSMaxB · 2017-02-23T10:07:37Z

Isn't it enough to specify this in the header where the function prototypes are instead of doing it in both the header and C file?

mjerris · 2017-02-23T14:31:46Z

unfortunately no, then you'll get declaration differs from definition type errors.

FSMaxB · 2017-02-26T12:44:10Z

cJSON.h

@@ -81,84 +81,113 @@ typedef struct cJSON_Hooks
      void (*free_fn)(void *ptr);
 } cJSON_Hooks;



Please add a comment that explains this.

explains what exactly?

What the export does. And why it is needed for DLLs.

gotcha. The comment was above that block so in github UI you can't see what you are referring to. Sure, no problem with this.

FSMaxB · 2017-02-26T12:45:31Z

cJSON.h

+#if defined(CJSON_DECLARE_STATIC)
+#define CJSON_DECLARE(type)			type __stdcall
+#define CJSON_DECLARE_NONSTD(type)	type __cdecl
+#define CJSON_DECLARE_DATA


Remove CJSON_DECLARE_NONSTD and CJSON_DECLARE_DATA. (also from other branches of the if)

yeah these are not used here.. leftovers from other places i've used similar approach. I'll remove.

FSMaxB · 2017-02-26T12:46:25Z

cJSON.h

+#endif
+#else /* !WIN32 */
+#if (defined(__GNUC__) || defined(__SUNPRO_CC) || defined (__SUNPRO_C)) && defined(CJSON_API_VISIBILITY)
+#define CJSON_DECLARE(type)		    __attribute__((visibility("default"))) type


Does it make sense to declare the "default" visibility? If not, please remove.

FSMaxB · 2017-02-26T12:47:20Z

cJSON.h

+#endif
+#ifdef __WINDOWS__
+#if defined(CJSON_DECLARE_STATIC)
+#define CJSON_DECLARE(type)			type __stdcall


Is the __stdcall necessary? If not, please make CJSON_DECLARE a macro that doesn't take any parameters.

Also rename CJSON_DECLARE to CJSON_PUBLIC

I know it used to be required. I'll test if it defaults sanely now. I think it defaults to cdecl, which may be fine in this case. I'll review to confirm. It would be much nicer to be able to do this without needing the arg in the macro. I know in the past this didn't work. Are you ok if it works on modern compilers but not older ones?

If it is not possible to do this on old compilers otherwise, than keep it as it is.

FSMaxB · 2017-02-26T12:48:38Z

cJSON.h

+#define CJSON_DECLARE(type)			type __stdcall
+#define CJSON_DECLARE_NONSTD(type)	type __cdecl
+#define CJSON_DECLARE_DATA
+#elif defined(CJSON_EXPORTS)


Rename CJSON_EXPORTS to CJSON_EXPORT_SYMBOLS.

FSMaxB · 2017-02-26T12:50:15Z

Sorry for taking so long to respond.

First of all thank you for putting you time and effort into cJSON.

I will accept this change, but I would like some changes:

Please redo the Pull request to the develop branch
Please add the same to cJSON_Utils
Please make the changes requested in my comments.

If possible, add support to the build system, but I can do it, if you are not comfortable with cmake. There should be an option for CJSON_EXPORT_SYMBOLS. You can also use CMake in order to find out if it was statically or dynamically compiled.

mjerris · 2017-02-27T17:27:34Z

in regards to make changes... its a bit tricky to do this without something like autoconf i can pass args to. I could make a bunch more targets for different combinations, but in pure make this ends up being a bit messy and some things would require conditions that are gnu make specific. What exactly did you have in mind. for example, it makes sense to by default use visibility where you can in the case of building dynamic, but building static, you may or may not want to have symbols exported based on usage (an example use case would be where you are building static for linking into another dynamic lib, but you may or may not want the symbols exported out of that lib).

FSMaxB · 2017-02-27T17:28:46Z

Don't worry about pure make. In this case you can just rely on the user passing in the defines manually.

Just CMake.

mjerris · 2017-02-27T19:07:26Z

I think this covers all the change requests. Let me know what else you see.

FSMaxB · 2017-02-27T19:42:09Z

cJSON.h

+
+/* When compiling for windows, we specify a specific calling convention to avoid issues where we are being called from a project with a different default calling convention.  For windows you have 2 define options:
+
+CJSON_DECLARE_STATIC - Define this in the case where you don't want to ever dllexport symbols


I'm not sure if I understand correctly. But this is not related to building static or dynamic libraries, right?

In this case I think CJSON_HIDE_SYMBOLS would be a better name and it could also set the visibility to hidden for the *nix compilers. Or did I misunderstand that?

it is not directly related to building static or dynamic, it has to do with which symbols will be exported from the dll. if you set that define, it would export no symbols, but lets say you built static then linked it into another dll, you could use the symbols inside the dll, but would not be able to use them on the outside of the dll. agree with the name change.

pushed that change

…dllimport/dllexport

mjerris · 2017-02-27T19:56:39Z

can you clarify what the different branches are and where this patch is targeted to?

FSMaxB · 2017-02-27T20:05:21Z

I haven't really formalised it.

v1 was originally intended for fixes once version 2 is out. But currently it doesn't really serve any purpose.
v2 is just the branch where I do breaking changes for version 2, it has been and will be rebased frequently, don't base anything off that
develop was just added recently and I plan to use this branch for all the development work that will go into the next release.
master is now intended to always be the same as the latest release, or contain small bugfixes that will increase the third number of the version number.

I plan to release version 1.4.0 including your changes and what I did on the develop branch.

FSMaxB · 2017-02-27T20:06:00Z

I should add a CONTRIBUTING.md, but I haven't come around to write one.

mjerris · 2017-02-27T20:20:41Z

sounds good.. does it matter that the other commit in master (the buffer overrun one) is not in develop branch right now, or develop will merge back to master pre-release?

FSMaxB · 2017-02-27T20:36:07Z

No problem, git handles merges like that quite well. And if it doesn't it's my job to fix it. You should never try to prevent merge conflicts by duplicating a commit so that it exists in both branches.

mjerris · 2017-02-27T20:37:30Z

yep, get that, just didn't know how your workflow works. Okay. I think you have it all from me now. Let me know if you need anything else.

FSMaxB · 2017-02-27T20:51:47Z

Looks good to me. I will merge this now.

FSMaxB · 2017-02-27T20:53:29Z

Thank you very much for your contribution to cJSON.

FSMaxB · 2017-02-27T20:54:07Z

One additional question: Would it be a sensible default to always set CJSON_EXPORT_SYMBOLS on windows?

mjerris · 2017-02-27T20:57:14Z

from the Makefile, yes, from the header file no... you want it to hit the dllimport branch of that #if for people using the lib and the dllexport branch when building the lib itself

FSMaxB · 2017-02-27T20:57:59Z

Not the makefile, but CMake.

FSMaxB · 2017-02-27T21:01:29Z

Maybe the makefile as well.

mjerris · 2017-02-27T21:03:51Z

it makes sense in cmake to add flags. to choose which. For shared lib, defaulting to export on windows makes sense. For static lib, its arguable what the better default should be. On the *nix side, something similar makes sense. I do a check if visibility is available, and if so, set the cflags for hidden, then set the define to expose the symbols (again on shared lib that makes sense, on static, its arguable what the default would be)

mjerris · 2017-02-27T21:04:46Z

you can see what i do in freeswitch here (its autoconf but the basic idea should be clear enough) .. search for visibility in this file:

https://freeswitch.org/stash/projects/FS/repos/freeswitch/browse/configure.ac

FSMaxB · 2017-02-27T21:27:25Z

Ok, thanks. I think I will set defaults when building as shared lib, otherwise don't do anything if no CMake option was set.

FSMaxB added the needs info label Feb 22, 2017

FSMaxB requested changes Feb 26, 2017

View reviewed changes

FSMaxB added enhancement and removed needs info labels Feb 26, 2017

mjerris force-pushed the fschanges branch from 5818645 to f0f5297 Compare February 27, 2017 19:06

mjerris changed the base branch from master to develop February 27, 2017 19:06

FSMaxB reviewed Feb 27, 2017

View reviewed changes

add CJSON_PUBLIC macro to public functions to support visibility and …

039b1cc

…dllimport/dllexport

mjerris force-pushed the fschanges branch from f0f5297 to 039b1cc Compare February 27, 2017 19:47

FSMaxB merged commit 024f690 into DaveGamble:develop Feb 27, 2017

		@@ -81,84 +81,113 @@ typedef struct cJSON_Hooks
		void (free_fn)(void ptr);
		} cJSON_Hooks;


		/* When compiling for windows, we specify a specific calling convention to avoid issues where we are being called from a project with a different default calling convention. For windows you have 2 define options:

		CJSON_DECLARE_STATIC - Define this in the case where you don't want to ever dllexport symbols

add CJSON_DECLARE macros to support gcc visibility and windows dllexport #116

add CJSON_DECLARE macros to support gcc visibility and windows dllexport #116

Conversation

mjerris commented Feb 22, 2017

FSMaxB commented Feb 22, 2017

mjerris commented Feb 23, 2017

FSMaxB commented Feb 23, 2017

mjerris commented Feb 23, 2017

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

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

FSMaxB commented Feb 26, 2017 • edited Loading

mjerris commented Feb 27, 2017

FSMaxB commented Feb 27, 2017 • edited Loading

mjerris commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjerris commented Feb 27, 2017

FSMaxB commented Feb 27, 2017

FSMaxB commented Feb 27, 2017

mjerris commented Feb 27, 2017

FSMaxB commented Feb 27, 2017

mjerris commented Feb 27, 2017

FSMaxB commented Feb 27, 2017

FSMaxB commented Feb 27, 2017

FSMaxB commented Feb 27, 2017

mjerris commented Feb 27, 2017 • edited Loading

FSMaxB commented Feb 27, 2017

FSMaxB commented Feb 27, 2017

mjerris commented Feb 27, 2017

mjerris commented Feb 27, 2017

FSMaxB commented Feb 27, 2017

FSMaxB commented Feb 26, 2017 •

edited

Loading

FSMaxB commented Feb 27, 2017 •

edited

Loading

mjerris commented Feb 27, 2017 •

edited

Loading