Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portability improvements, cleanup, and bug fixes #36

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

johnsonjh
Copy link
Contributor

@johnsonjh johnsonjh commented Aug 7, 2024

  • Portability improvements, cleanup, and bug fixes:
    • Add or fix support for building on Solaris, illumos, Linux/musl, NetBSD, OpenBSD, IBM AIX, IBM OS/400 (PASE), Cygwin, and Haiku, which weren't previously cleanly building.
    • Use pkg-config when available to determine CFLAGS and libraries.
    • Initialize some variables in supdupd.
    • Fix off-by-one error in supdup.
    • Suppress spurious error when Chaosnet bridge socket is missing.
    • Create README.md.
    • Cleanup Makefile.
    • Cleanup excess trailing whitespace characters.
    • Indent nested preprocessor directives with GNU Cppi.
    • Consistently use modern preprocessor syntax.
    • Properly detect bad ports (negative or >65535).
    • Fix non-ANSI function declaration (put_newline).
    • Add *.ln (Lint) files, supdupd, and compile_commands.json to .gitignore.
    • Create .gitattributes.
    • Ensure file handle is valid (non-negative) before closing.
    • Correct various spelling errors.

  • Tested build under Oracle Solaris, OpenIndiana illumos, Linux/musl, Linux/glibc, IBM AIX, IBM OS/400 (PASE), Haiku, FreeBSD, OpenBSD, NetBSD, Cygwin, and Apple macOS.

  • Compilation tested and working with PCC, GCC, Clang, Xcode, IBM XL C, IBM Open XL C, Oracle Studio C, NVIDIA HPC SDK C, Portland Group C, and DMD ImportC.

  • Passes Cppcheck and Clang Analyzer.

Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>

GNUmakefile Outdated Show resolved Hide resolved
GNUmakefile Outdated Show resolved Hide resolved
GNUmakefile Outdated Show resolved Hide resolved
@@ -4,33 +4,38 @@

#if USE_CHAOS_STREAM_SOCKET

#include <stdio.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this is a but superfluous? I have no opinion though ..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's just GNU style (modifications via GNU Cppi tool). If you hate it, I can revert.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that this was resolved in the discussions. I think minor whitespace cleanup is ok, but major reformatting is not.

Copy link
Contributor Author

@johnsonjh johnsonjh Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That change is from GNU Cppi which normalizes preprocessor indentation to GNU style - it's indenting by one space because those preprocessor directives are inside the conditional #if block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but we haven't adopted GNU style here. At least it hasn't been discussed and agreed on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert to the previous flat style with no indentation (all preprocessor directives left justified) if it's preferred. I apologize, it's something that I (and my default editor configuration) do automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, need to apologize! We have these reviews and discussions to hash things out.

In my view, reformatting be more nuisance than it's worth. Say, if someone patched a fork and wanted to apply it here later. It's also polite to adopt the existing code style. So that's my preference, but I'm not saying I have the final say.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll still lean to the new formatting due to the (IMO, massive) readability improvements, but let me know either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @larsbrinkhoff -- one benefit of keeping formatting is that when you compare to old versions of supdup that might surface it is much much easier to find changes.

As for GNU cppi .. I personally find it ugly, and it isn't that commonly adopted in GNU.

supdup.c Outdated Show resolved Hide resolved
chaos.c Outdated Show resolved Hide resolved
chaos.c Outdated Show resolved Hide resolved
@johnsonjh

This comment was marked as outdated.

@johnsonjh

This comment was marked as outdated.

@johnsonjh johnsonjh force-pushed the 20240807/johnsonjh/portability branch from 73bbd67 to ff2c6ee Compare August 9, 2024 00:43
* Add or fix support for building on Solaris, illumos,
  Linux/musl, NetBSD, OpenBSD, IBM AIX, and Haiku.
* Use pkg-config when available to determine CFLAGS and
  libraries.
* Cleanup Makefile.
* Cleanup excess trailing whitespace characters.
* Indent nested preprocessor directives with GNU Cppi.
* Tested build under Oracle Solaris, OpenIndiana illumos,
  Linux/musl, Linux/glibc, IBM AIX, Haiku, FreeBSD, OpenBSD,
  NetBSD, and Apple macOS.
* Compilation tested and working with PCC, GCC, Clang, IBM XL C,
  IBM Open XL C, Oracle Studio C, NVIDIA HPC SDK C, Portland
  Group C, and DMD ImportC.

Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
supdup.c:1157:6: error: Array 'loc[128]' accessed at index 128, which is out of bounds. [arrayIndexOutOfBounds]
  loc[i] = '\0';  /* terminate */
     ^
supdup.c:1136:3: note: After for loop, i has value 128
  for (i = 0; i < sizeof(loc); i++) {
  ^
supdup.c:1157:6: note: Array index out of bounds
  loc[i] = '\0';  /* terminate */
     ^

Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
On every system I've tested, errno is set to ENOENT rather than
ECONNREFUSED.  This change successfully suppreses the spurious
"connect(server): No such file or directory" error message.

Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
@johnsonjh

This comment was marked as outdated.

@johnsonjh johnsonjh changed the title Portability enhancements Portability enhancements and bug fixes Aug 9, 2024
@johnsonjh johnsonjh changed the title Portability enhancements and bug fixes Portability improvements, cleanup, and bug fixes Aug 9, 2024
@johnsonjh johnsonjh force-pushed the 20240807/johnsonjh/portability branch from d6161f6 to d884203 Compare August 9, 2024 07:26
Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
supdup.c:191:19: warning: non-ANSI function declaration of function 'put_newline'

Found with smatch.

Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
@johnsonjh johnsonjh force-pushed the 20240807/johnsonjh/portability branch from 6df18ea to f83521c Compare August 9, 2024 10:34
Exclude `doc/` from GitHub Linguist; resolves 48.9% of
the project being detected (incorrectly) as CartoCSS.

Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a README.md Maybe necessary to link with tinfo? Compilation failure on OpenBSD
3 participants