Compilation errors building 4.2.0 w/ VS 2019

Collapse
X
 
  • Time
  • Show
Clear All
new posts
  • eastwind
    Apprentice
    • Dec 2019
    • 79

    Compilation errors building 4.2.0 w/ VS 2019

    I got 4.2.0 to build under VS 2019, but not without a few code fixes. FWIW I was building on win 10 with the community (free) edition of VS, no cygwin installed with my own solution & vcxproj.

    Per the FAQ, I'm reporting them in a thread (I think the idea is they aren't 'bugs' until someone else hits them). Since they're all compilation errors I'm going to bundle them in one thread rather than spam 4 threads.

    ****** Bug #1).

    C1189: #error: Macro definition of snprintf conflicts with Standard Library function declaration

    Lots of .c files have this error, any that include h-basic.h. Line 29 defines snprintf because old versions of MSC needed a definition. But at some point (*) they fixed the compiler, and now defining the macro causes both a C4005 macro redefinition warning and a C1189 error. The problem lines in h-basic.c are these:

    /**
    * Native MSVC compiler doesn't understand inline or snprintf
    */
    #ifdef _MSC_VER
    # define inline __inline
    # define snprintf _snprintf
    #endif

    My fix was to restrict those lines to early versions of Visual C. I *think* the change happened with VS 2015, so I changed the #ifdef to:

    #if (defined(_MSC_VER) && _MSC_VER < 1900L)

    1900L is supposed to be VS 2015, but I don't have VS 2015 or an earlier version to test this fix. This change should only affect anybody compiling with VS 2015 or later.

    An alternate fix would be to officially decide that only recent versions of MS C are supported, and get rid of the whole snippet. I believe the issue is that for a long time MS C did not support C99, but finally added some support in VS 2015, and the Angband code assumes C99 and uses snprintf and the inline keyword. The snippet was a fix for MS C not supporting that portion of C99, but now it does, so the snippet will cause an error on C99-supporting versions of MS C.

    **** Bug #2):

    load.c(852,36): error C2057: expected constant expression
    load.c(852,36): error C2466: cannot allocate an array of constant size 0
    load.c(852,37): error C2133: 'itypes': unknown size

    Line 852 is the declaration:

    bitflag flags, itypes[itype_size];

    itype_size is a local variable. You can't allocate dynamic-sized arrays on the stack in C like you can in C++. My fix was to allocate the max size that itype_size is allowed to have all the time, which is the #define constant
    ITYPE_SIZE - it's not very large, and this array only sits on the stack for a short time anyway.

    bitflag flags, itypes[ITYPE_SIZE];

    An alternative fix would be to use the C++ compiler - but that might have a lot of consequences.

    ***** Bug #3):

    scrnshot.c(105): error C4703: potentially uninitialized local pointer variable 'info_ptr' used

    I mentioned this in another thread. I believe it's actually a compiler bug, not an Angband bug, but I know from experience that reporting it to Microsoft will result in them replying "well don't write screwy code", because the C-comipiler front end is a different code base for them than their C++ front end, and the C front end is very old code (perhaps dating from the 80's) and they touch it as little as possible.

    My workaround is to initialize info_ptr to null in its declaration on line 40:

    png_infop info_ptr = NULL;

    Of course rewriting the routine wouldn't be a bad idea, but adding '= NULL' is the minimum to get the code to compile.

    ***** Bug #4):

    z-file.c(389,1): error C4996: 'open': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _open. See online help for details.
    corecrt_io.h(516): message : see declaration of 'open'

    I think this is another compiler change that came in with VS 2015 and C99 support. In this case the fix is to add some lines for versions 2015 and after. I added these lines to z-file.c after line 55:

    /* Suppress MSC C4996 error */
    #if defined(_MSC_VER) && (_MSC_VER >= 1900L)
    #define open _open
    #define fdopen _fdopen
    #define mkdir _mkdir
    #endif

    (I only pasted one C4996 error above, but z-file.c gets several, for the three functions redefined, so all 3 defines are needed).

    *****

    That's it - I get a build with those fixes, plus making sure not to define _UNICODE and adding a couple other defines on the command line. I've tested my build for a couple hours and it is stable and shows no issues, but for completeness somebody with an early MS C should compile these fixes, and somebody with cygwin should make sure they don't have any effect for cygwin.
  • eastwind
    Apprentice
    • Dec 2019
    • 79

    #2
    Is there any interest in having these fixes pushed? I can try to figure out how to use Git if there is interest, but if no one else is trying to us VS I won't bother.

    Before I do submit them, I would need some feedback on a couple of the #ifdefs - whether I've done them right so as to not mess up other builds.

    Comment

    • Nick
      Vanilla maintainer
      • Apr 2007
      • 9647

      #3
      Originally posted by eastwind
      Is there any interest in having these fixes pushed? I can try to figure out how to use Git if there is interest, but if no one else is trying to us VS I won't bother.

      Before I do submit them, I would need some feedback on a couple of the #ifdefs - whether I've done them right so as to not mess up other builds.
      Yes, there definitely is, and just listing your changes would be fine (although obviously if you want to learn git go right ahead).
      One for the Dark Lord on his dark throne
      In the Land of Mordor where the Shadows lie.

      Comment

      • eastwind
        Apprentice
        • Dec 2019
        • 79

        #4
        Bug number 3 is already fixed in master by the fix for #4236, I couldn't find an issue for the other 3 so I opened #4266.

        Nick, sorry if #4266 is a dup. I am ready to put up a pull request with these fixes once I have the right issue to reference.

        Comment

        • Nick
          Vanilla maintainer
          • Apr 2007
          • 9647

          #5
          No it's not - I missed this thread altogether when I did #4236.
          One for the Dark Lord on his dark throne
          In the Land of Mordor where the Shadows lie.

          Comment

          • eastwind
            Apprentice
            • Dec 2019
            • 79

            #6
            Ok, I think I stumbled my way through all the Git malarky and there's a pull request waiting your approval. It looks kind of like creating the pull request created a new issue #4267 referencing #4266 that I thought I was fixing.

            Anyway, see the comments for a description of a simpler version of the fix that deprecates VS compilers prior to 2015. The fixes I pushed don't make that decision.

            Comment

            • Nick
              Vanilla maintainer
              • Apr 2007
              • 9647

              #7
              Malarky indeed. Apparently issues and pull requests use the same numbering system; and I knew if you types "Fix #nnn" or "Fixes #nnn" in a commit message it would close issue #nnn, but apparently this also works in the title of a pull request.
              One for the Dark Lord on his dark throne
              In the Land of Mordor where the Shadows lie.

              Comment

              • takkaria
                Veteran
                • Apr 2007
                • 1951

                #8
                Originally posted by eastwind
                You can't allocate dynamic-sized arrays on the stack in C like you can in C++.
                FWIW that isn't true. C99 allows variable-sized arrays allocated on the stack. It's incredibly frustrating that Microsoft can't implement full support for a 20 year old standard!
                takkaria whispers something about options. -more-

                Comment

                • Pete Mack
                  Prophet
                  • Apr 2007
                  • 6883

                  #9
                  And yeah, MS doesn't want to bother with C, but it isn't that big a deal to make minor changes to a compiler front-end. (The back end is shared across languages.)

                  Comment

                  • eastwind
                    Apprentice
                    • Dec 2019
                    • 79

                    #10
                    The changes in the OP are now all in master.

                    I'll punt on the language feature support discussion.

                    Comment

                    Working...
                    😀
                    😂
                    🥰
                    😘
                    🤢
                    😎
                    😞
                    😡
                    👍
                    👎