improved detection of POSIX #489

Merged
merged 21 commits into from Dec 21, 2016

Projects

None yet

4 participants

@inikep
Contributor
inikep commented Dec 16, 2016

No description provided.

programs/dibio.c
@@ -12,7 +12,8 @@
/*-*************************************
* Includes
***************************************/
-#include "util.h" /* Compiler options, UTIL_GetFileSize, UTIL_getTotalFileSize */
+#include "platform.h" /* Compiler options */
@Cyan4973
Cyan4973 Dec 17, 2016 Collaborator

Since platform.h is already included at top of util.h, is there any benefit at including it again in each user source file of util.h ?

@inikep
inikep Dec 17, 2016 Contributor

It cleary shows what we need platform.h for.

+ */
+
+#ifndef PLATFORM_H_MODULE
+#define PLATFORM_H_MODULE
@Cyan4973
Cyan4973 Dec 17, 2016 Collaborator

Could you define (in a comment section) what's the objective of this file ?

@inikep
inikep Dec 17, 2016 Contributor

I divided util.h into two files util.h and platform.h. Now we can use compiler macros (with POSIX macros) without functions declared in util.h. It's already used in datagen.
More was described here:
https://app.asana.com/0/91843333395082/231365028765940

lib/zstd.h
#else
-# define ZSTDLIB_API
+# define ZSTDLIB_API ZSTDLIB_VISIBILITY
@Cyan4973
Cyan4973 Dec 17, 2016 edited Collaborator

What's the point of ZSTDLIB_VISIBILITY ?
Why does it help ?

@terrelln
terrelln Dec 17, 2016 Contributor

I think ZSTDLIB_VISIBILITY is only a helper macro. MinGW needs both __attribute__((visibility("default"))) and __declspec(dll{import, export}).

@inikep
inikep Dec 17, 2016 Contributor

Nick is right.
It solves a bug with __attribute__ ((visibility ("default"))) and broken DLL compilation with gcc/MinGW

@Cyan4973
Collaborator

OK, it seems there is a need to rebase to get rid of some conflict

@inikep inikep Merge remote-tracking branch 'refs/remotes/facebook/dev' into v112
# Conflicts:
#	appveyor.yml
f8046b8
@inikep
Contributor
inikep commented Dec 19, 2016

The other solutions I can see for separate compiler macros and functions:
a) use util.c (functions definitions; don't declare functions as static) and util.h (compiler macros and functions declarations)
b) leave a single file util.h and use #define UTIL_STATIC_LINKING_ONLY when needed

@Cyan4973
Collaborator

Actually, I like platform.h, as it conveys the essential concept that we are dealing with platform-specific code.

I'm still puzzled about the benefit of having 2 files, since datagen is really a small secondary program, and if it's the only one to benefit from this separation, maybe it can be done differently. For example, like directly integrating relevant macros within datagencli.c.

@inikep
Contributor
inikep commented Dec 19, 2016

Datagen was only an example. I wanted to have separate compiler macros and utility functions. platform.h was intended to be included in all LZ4 and zstd executables. util.h should be included when needed but in practice is almost always included.

Then I wanted to move all Compiler Options and OS-specific Includes from lz4cli.c, lz4io.c, fileio.c, zstdcli.c to platform.h.
Advatanges:

  • easy maintenance - when we improve something it's done for all executables
  • readability - these macros look bad at the beginning of C files and should be put in .h file

An example:
Lately somebody added the following macros to lz4io.c.
#define _LARGE_FILES /* Large file support on 32-bits AIX /
#define _FILE_OFFSET_BITS 64 /
off_t width */
#define _LARGEFILE_SOURCE

It probably broken Solaris 64-bit with 32-bit executables which declares:

root@Solaris:/mnt/sf_D_DRIVE/Code/features# gcc feature.c -o fea && ./fea
__unix__ defined
__unix defined
_POSIX_VERSION defined: 199506L
_XOPEN_VERSION defined: 3L
_LARGEFILE64_SOURCE defined
_FILE_OFFSET_BITS defined: 32

Moreover these macros were already defined in util.h in a more portable way but there is too much stuff in util.h so it's not so clear.
Isn't it better just to #include "platform.h"?

inikep added some commits Dec 21, 2016
@inikep inikep improved util.h and platform.h ead350b
@inikep inikep executables use new util.h and platform.h
16ae656
@inikep inikep simplified zstdcli.c 20b089e
@inikep inikep fix basic types redefinition
5736db2
@inikep inikep improved MinGW support a35b944
programs/util.h
@@ -84,7 +84,9 @@ extern "C" {
/*-**************************************************************
* Basic Types
*****************************************************************/
-#if !defined (__VMS) && (defined (__cplusplus) || (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) /* C99 */) )
+#ifndef BASIC_TYPES_DEFINED
@Cyan4973
Cyan4973 Dec 21, 2016 Collaborator

The initial intention was to #include "mem.h" to access these definitions.

Why do you prefer to repeat them here, implying the need of an additional macro to avoid double-definitions ?

@inikep
inikep Dec 21, 2016 edited Contributor

This is just for LZ4. I've done similar changes for LZ4 and I want to use the same util.h and platform.h for both . If you'll decide to join util.h and platform.h it's also not a problem.

@Cyan4973
Cyan4973 Dec 21, 2016 Collaborator

I really prefer to keep the "mem.h" approach within zstd.
Unifying with LZ4 is a nice bonus, but a secondary one.

lz4 source has a different policy : the core library must remain "single file", with only "lz4.c" + "lz4.h", and no other dependency required (except standard lib), which is obviously completely difference from zstd source tree.

Now, lz4 cli is not the "core library".
Here, it would be possible to bring in mem.h if it helps.

@inikep inikep fixed Visual Studio compilation
101df4f
@Cyan4973
Collaborator
Cyan4973 commented Dec 21, 2016 edited

Isn't it better just to #include "platform.h"?

I believe that's the core point, one that needs to be explicitly presented.

I'm certainly not opposed to this change, but it's not "just a" patch. It has strong impacts on source code, dependency tree, roles and scope. So it needs to be properly discussed and reviewed.

lib/common/mem.h
@@ -46,6 +46,8 @@ MEM_STATIC void MEM_check(void) { MEM_STATIC_ASSERT((sizeof(size_t)==4) || (size
/*-**************************************************************
* Basic Types
*****************************************************************/
+#ifndef BASIC_TYPES_DEFINED
+#define BASIC_TYPES_DEFINED
@Cyan4973
Cyan4973 Dec 21, 2016 Collaborator

This macro now acts as a kind of "hidden interface contract" with mem.h, which spells trouble for future maintenance.
That's one reason I prefer it removed.

programs/platform.h
+* Compiler Options
+****************************************/
+#if defined(__INTEL_COMPILER)
+# pragma warning(disable : 177) /* disable: message #177: function was declared but never referenced */
@Cyan4973
Cyan4973 Dec 21, 2016 edited Collaborator

OK, here is one typical issue with defining such compilation flag within platform.h :
now, non-referenced function warning is disabled across a large portion of source code (at least, any source which includes directly or indirectly platform.h).

That could be a good idea if there was some functions within platform.h that could uselessly trigger it, but that's not the case here : I don't see any function declaration in platform.h, only macros, so the flag is enabled on behalf of future users, such as, maybe, util.h.

But now, a user which only includes platform.h has a useful warning flag disabled. This flag could help him detect bugs before they get more complex to understand.

Disabling a warning flag can be completely legitimate, but it's better done close to, and even limited to, the section where it's needed, and better avoided as a "global setting".

If the point here is to disable it for util.h, better do it within util.h (and even better, re-enable it at the end of the file). I suspect it's related to UTIL_STATIC, in which case this is the better place to define it.

@inikep
inikep Dec 21, 2016 edited Contributor

I moved it to util.h, next to a definition of UTIL_STATIC.

What about:
# pragma warning(disable : 4127)
Do you prefer to declare it in every .c file that needs it?

@Cyan4973
Cyan4973 Dec 21, 2016 edited Collaborator

Yes. Disabling compilation flag should be done on a scope as minimal as possible.

programs/platform.h
+# pragma warning(disable : 177) /* disable: message #177: function was declared but never referenced */
+#endif
+#if defined(_MSC_VER)
+# define _CRT_SECURE_NO_WARNINGS /* Disable some Visual warning messages for fopen, strncpy */
@Cyan4973
Cyan4973 Dec 21, 2016 Collaborator

same comment as above

+}
+#endif
+
+#endif /* PLATFORM_H_MODULE */
@Cyan4973
Cyan4973 Dec 21, 2016 Collaborator

The rest of the file makes sense :
it deals with system-specific adaptations, which has more to do with OS and / or compiler restrictions and abilities.

inikep added some commits Dec 21, 2016
@inikep inikep platform.h: removed Compiler Options 2f6ccee
@inikep inikep _CRT_SECURE_NO_WARNINGS moved to util.h
e679741
@inikep inikep updated comments 97a258d
programs/util.h
+ Copyright (C) 2016-present, Przemyslaw Skibinski, Yann Collet
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
@Cyan4973
Cyan4973 Dec 21, 2016 edited Collaborator

The license of util.h is changed from BSD to GPLv2.
Is that intentional ?

programs/platform.h
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
@Cyan4973
Cyan4973 Dec 21, 2016 Collaborator

We are supposed to use BSD license for Facebook Open-Source projects.
There can be exceptions, sure, but for a file which belongs to the core cli experience, it's likely not possible nor desirable to make such an exception.

@inikep
inikep Dec 21, 2016 Contributor

OK

@inikep
Contributor
inikep commented Dec 21, 2016

I moved

#  define _CRT_SECURE_NO_WARNINGS
#  define _CRT_SECURE_NO_DEPRECATE

to util.h (we need them for functions declared there) and it doesn't work when platform.h is defined before as it includes io.h.
Possible solutions:
a) keep _CRT_SECURE_NO_WARNINGS in platform.h
b) join platform.h with util.h

@inikep
Contributor
inikep commented Dec 21, 2016

The last commit uses 3rd solution that is define either platform.h or util.h but it's not a future proof solution.

@Cyan4973
Collaborator

My understanding is that the need for _CRT_SECURE_NO_WARNINGS macro is related to io.h, which is included in platform.h.
In which case, it seems fair to keep this macro in platform.h.

It seems to have an impact on #include <io.h>, so it's probably best to define it just before it.

@inikep
Contributor
inikep commented Dec 21, 2016

The last option that comes to my mind is to move definition of

#  define IS_CONSOLE(stdStream)
#  define SET_BINARY_MODE(file)
#  define SET_SPARSE_FILE_MODE(file)

to util.h but then platform.h will be very short.

@inikep inikep util.h: restore BSD license for Facebook Open-Source
7a8a03c
@inikep
Contributor
inikep commented Dec 21, 2016

It seems to have an impact on #include <io.h>, so it's probably best to define it just before it.

I think that any #include with Visual can cause later _CRT_SECURE_NO_WARNINGS to be ignored. I would prefer to keep it at the beginning for better readability and not to forget that these macros are already defined.

@Cyan4973 Cyan4973 merged commit 0d7e848 into facebook:dev Dec 21, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@inikep inikep deleted the inikep:v112 branch Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment