Compiler/Linker errors under MSC 16 (VS2010)
(1) By anonymous on 2021-01-14 11:04:44 [source]
Hello all,
I am new to fossil. I have been compiling the fossil src code with an older Microsoft compiler (MSC/C++ 16.00 / VS2010) under Win10 and have run into two compilation/linking errors of the trunk branch.
1 - C89 compliance: In import.c
and timeline.c
there are some variable declarations which are not at
the beginning of a code block, but intermingled with statements. I assume this should be corrected.
Here is a patch (against check-in c21f7397
):
Index: src/import.c
==================================================================
--- src/import.c
+++ src/import.c
@@ -1797,10 +1797,13 @@
gsvn.zBranches = find_option("branches", 0, 1);
gsvn.zTags = find_option("tags", 0, 1);
gsvn.revFlag = find_option("rev-tags", 0, 0)
|| (incrFlag && !find_option("no-rev-tags", 0, 0));
}else if( gitFlag ){
+ const char *zGitUser;
+ char *currGitUser;
+
markfile_in = find_option("import-marks", 0, 1);
markfile_out = find_option("export-marks", 0, 1);
if( !(ggit.zMasterName = find_option("rename-master", 0, 1)) ){
ggit.zMasterName = "master";
}
@@ -1807,15 +1810,15 @@
ggit.authorFlag = find_option("use-author", 0, 0)!=0;
/*
** Extract --attribute 'emailaddr username' args that will populate
** new 'fx_' table to later match username for check-in attribution.
*/
- const char *zGitUser = find_option("attribute", 0, 1);
+ zGitUser = find_option("attribute", 0, 1);
while( zGitUser != 0 ){
ggit.gitUserInfo = fossil_realloc(ggit.gitUserInfo, ++ggit.nGitAttr
* sizeof(ggit.gitUserInfo[0]));
- char *currGitUser = fossil_strdup(zGitUser);
+ currGitUser = fossil_strdup(zGitUser);
ggit.gitUserInfo[ggit.nGitAttr-1].zEmail = next_token(&currGitUser);
ggit.gitUserInfo[ggit.nGitAttr-1].zUser = rest_of_line(&currGitUser);
zGitUser = find_option("attribute", 0, 1);
}
}
Index: src/timeline.c
==================================================================
--- src/timeline.c
+++ src/timeline.c
@@ -2982,15 +2982,16 @@
}else{
zFree = mprintf("[%S] %s%s", zId, zPrefix, zCom);
}
if( zFormat ){
+ char *zEntry;
+ int nEntryLine = 0;
+
if( nChild==0 ){
sqlite3_snprintf(sizeof(zPrefix)-n, &zPrefix[n], "*LEAF* ");
}
- char *zEntry;
- int nEntryLine = 0;
zEntry = timeline_entry_subst(zFormat, &nEntryLine, zId, zDate, zUserShort,
zComShort, zBranch, zTags, zPrefix);
nLine += nEntryLine;
fossil_print("%s\n", zEntry);
fossil_free(zEntry);
2 - Compatibility: In pikchr.c
I ran into the fact that snprintf()
and rint()
are not available in the MSVC libraries prior to version 19 (VS2015).
For snprintf()
I added a #define
to map to _snprintf()
(which is available) similar to what was done in other fossil sources. For _rint()
I added a local workaround definition. pikchr
appears to work well for me with this workaround, but I did not do extensive (structured) testing. A patch against check-in c21f7397
is below.
Grateful for suggestions on how to proceed, including other/better solutions.
Index: src/pikchr.c
==================================================================
--- src/pikchr.c
+++ src/pikchr.c
@@ -124,10 +124,14 @@
#include <assert.h>
#define count(X) (sizeof(X)/sizeof(X[0]))
#ifndef M_PI
# define M_PI 3.1415926535897932385
#endif
+
+#if defined(_MSC_VER) && (_MSC_VER < 1900)
+# define snprintf( s, len, ...) { _snprintf( s, (len), __VA_ARGS__); s[(len)-1] = 0; }
+#endif
/* Tag intentionally unused parameters with this macro to prevent
** compiler warnings with -Wextra */
#define UNUSED_PARAMETER(X) (void)(X)
@@ -6539,10 +6543,28 @@
case T_LEFT: v = pObj->bbox.sw.x; break;
case T_RIGHT: v = pObj->bbox.ne.x; break;
}
return v;
}
+
+/* Local rint() replacement for MSVC < 2015
+*/
+#if defined(_MSC_VER) && (_MSC_VER < 1900)
+# if defined(_WIN64)
+# include <emmintrin.h>
+ static double rint(double const x) {
+ return (double)_mm_cvtsd_si32(_mm_load_sd(&x));
+ }
+# else
+ static double rint(double x) {
+ __asm {
+ fld x
+ frndint
+ }
+ }
+# endif
+#endif
/* Compute one of the built-in functions
*/
static PNum pik_func(Pik *p, PToken *pFunc, PNum x, PNum y){
PNum v = 0.0;
(2.1) By Stephan Beal (stephan) on 2021-01-14 11:18:17 edited from 2.0 in reply to 1 [link] [source]
C89 compliance: In import.c and timeline.c ...
Thank you for the report.
The C99-isms are now resolved. The pikchr changes have been left for someone who has Windows and can try them out. (Edit: noting that the upstream copy of that file is in the pikchr project.)
(3) By anonymous on 2021-01-14 12:07:18 in reply to 2.1 [link] [source]
Thank you! Looking forward to the comments on pikchr.
(4) By anonymous on 2021-01-14 13:00:04 in reply to 2.1 [link] [source]
Kinda amazing we're pessimizing the code for a 32 years old standard, when you think of it :)
For using valid constructs in a 22 years old newer one.
As if the compiler can't figure out itself the stack space for locals in a function!
(5) By Stephan Beal (stephan) on 2021-01-14 13:10:56 in reply to 4 [link] [source]
Kinda amazing we're pessimizing the code for a 32 years old standard
"If it ain't broke, don't fix it."
This project conforms to C89 insofar as possible, barring the obligatory platform-specific code here and there and a dependency on the non-C89-standard-but-ubiquitious long long
integer type for 64-bit ints (first standardized in C99, but...). C99, unfortunately, was not properly supported by MS until after 2010. There's nothing which C99 and later provide, above and beyond C89, which we need nor which would help us significantly.
There are no compelling reasons for this project not to stick with C89 compatibility.
(6) By Warren Young (wyoung) on 2021-01-14 13:39:57 in reply to 5 [link] [source]
C99, unfortunately, was not properly supported by MS until after 2010.
2015, in fact, or 2019 if you insist on including the C99 preprocessor extensions as well. (Source)
However, that was the last good reason to keep insisting on sticking to C89 constructs, IMHO. Visual Studio Community is freely-available, and all other platforms include C99 conforming C compilers, even badly outdated ones like Solaris 11.
Unless someone can point out a platform Fossil currently runs on which only provides C89 with its normal C compiler, I think we should give up on this insistence that Fossil use only C89 constructs.
I can see how SQLite can still justify this restriction, but Fossil?
(7) By Daniel Dumitriu (danield) on 2021-01-14 13:50:16 in reply to 4 [link] [source]
I see no "pessimization" here, the changes are guarded so they don't have any effect on platforms and compilers other than MSC 16.00...
I think the question is once again the cost-effectiveness or tradeoff: how much effort for how much utility. In this particular case it looks positive - the effort is minimal. Admittedly, the utility is also small.
(8) By Stephan Beal (stephan) on 2021-01-14 13:51:48 in reply to 6 [link] [source]
I can see how SQLite can still justify this restriction, but Fossil?
My internal Devil's Advocate wants to know if there's a single C99 feature which would give us some capability we don't already have, or which would significantly simplify something we do? Being able to declare local vars at will doesn't quite qualify as significant improvement - minor syntactic sugar, at best.
"If it ain't broke..."
(9) By Daniel Dumitriu (danield) on 2021-01-14 13:55:42 in reply to 6 [link] [source]
As soon as complying with C89 stands in the way, I'd say we might think about moving forward. At the moment I don't think moving declarations is that cumbersome.
Which C99 (or C11?) constructs would bring value to the project? This is a real question.
(10) By Daniel Dumitriu (danield) on 2021-01-14 14:57:29 in reply to 3 [link] [source]
Pikchr is somebody's child, so hope for comments from there.
I have no MSVC older than 1916 around, it does compile (for 64-bit) and run fossil pikchr
with that patch when excluding the version guard, but I can say no more. The workaround for snprintf()
seems correct, no idea about rint()
.
On another level - I find "we're pessimizing" pretty funny, coming from the well-known anonymous.
(11) By Warren Young (wyoung) on 2021-01-14 16:05:32 in reply to 8 [link] [source]
The features of C99 I care about most are:
Portable fixed-size integer types. Although SQLite and Fossil have typedefs for these already, it's nice to be able to use the same types everywhere. If I want a 64-bit unsigned int, it's easier to remember
uint64_t
than to work out whatever equivalent the given software package I'm hacking on provides instead.Moving variable declarations to as close to their point of use has many benefits:
- It reduces the possibility of certain classes of bugs, such as variable shadowing in large functions, of which there are plenty in Fossil.
- It avoids uninitialized variables in the common pattern where you don't know the value of a variable until you're well into the function. Why declare the variable before it can safely be used? Declaring and defining your variables at their first point of use has known benefits, particularly in combination with
const
. - It can allow the compiler to do a better optimization job, since certain code paths won't require expensive initializations.
This is about more than just putting variable declarations "everywhere," since it also affects control structures:
if (int foo = bar()) { ...
…restricts
foo
to the "if" branch, something you can't readily do with C89 syntax.Standardization of common structure syntax shortcuts such as designated initializer syntax:
struct point p = { .x = 1, .y = 2 };
Fossil will either be avoiding those to conform to C89 or implicitly depending on C99 support by using them, as we already do withlong long
.Proper boolean type. No more
int not_quite_a_bool = 1
, which could just as well be assigned "5" or used in an arithmetic expression. I don't know whether C99 compilers commonly do enforce bool-ness when you usebool
, but if not, they can and should. C++ compilers do, so to the extent that most C99 compilers share code with a C++ compiler, I'd expect the same rules to be enforced.Variadic macros. Not often needed, but when you need one and can't use this feature because you're restricted to C89, the alternatives suuuuuck.
…And the trump card: it stops this repeated whinging. :)
One feature of C99 I've never found any good use for, but which drh may enjoy is restrict
.
(12) By Warren Young (wyoung) on 2021-01-14 17:32:01 in reply to 11 [link] [source]
if (int foo = bar()) { ...
A common case where you'd care about the improvement is:
if (const Thingy* pThingy = get_the_thingy()) {
use_the_thingy(pThingy->foo);
free(pThingy);
}
else {
cope_with_lack_of_thingy();
}
By following that coding style, you eliminate the possibility of:
- a null pointer dereference;
- a use-after-free bug; and
- reduce the scope of a potential memory leak
(13) By Richard Hipp (drh) on 2021-01-14 18:55:16 in reply to 1 [link] [source]
I do not have access to an historic version of MSVC in order to test out the proposed changes. And we don't have a Contributors License Agreement from the submitter. Hence, I'm inclined to omit the suggested changes to Pikchr. I'm OK with requiring a more recent version of MSVC in order to compile Fossil and/or Pikchr.
(14.1) By Warren Young (wyoung) on 2021-01-15 16:41:33 edited from 14.0 in reply to 6 [link] [source]
Unless someone can point out a platform Fossil currently runs on which only provides C89 with its normal C compiler
We have a ruling from chat: drh needs current versions of Fossil to run on ancient and oddball systems for testing the current version of SQLite on those platforms, since the alternatives are worse:
Keep using old Fossil versions on those platforms, since any future backward-incompatible changes (e.g. SHA-3, the recent EVENT table improvements...) would prevent current Fossil from building if we started using C99 constructs that won't compile on those systems
Go through a "scp the amalgam" type process to get current SQLite over to those platforms for testing, with the consequence that any needed changes have to be done on another platform and go 'round the loop again, rather than be checked in directly from the platform needing the changes.
Until SQLite no longer has to be tested and maintained on such platforms, Fossil has to build and run on them, too.