tuxji commented on a change in pull request #545:
URL: https://github.com/apache/daffodil/pull/545#discussion_r624139525
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libcli/daffodil_main.c
##########
@@ -15,13 +15,13 @@
* limitations under the License.
*/
-#include <stdio.h> // for NULL, perror, FILE, fclose, fflush, fopen,
stdin, stdout
-#include <string.h> // for strcmp
-#include "daffodil_argp.h" // for daffodil_parse, daffodil_parse_cli,
daffodil_unparse, daffodil_unparse_cli, daffodil_cli, parse_daffodil_cli,
DAFFODIL_PARSE, DAFFODIL_UNPARSE
-#include "errors.h" // for continue_or_exit, Error, print_diagnostics,
PState, UState, ERR_FILE_CLOSE, ERR_FILE_FLUSH, ERR_FILE_OPEN,
ERR_INFOSET_READ, ERR_INFOSET_WRITE
-#include "infoset.h" // for walkInfoset, InfosetBase, rootElement, ERD,
VisitEventHandler
-#include "xml_reader.h" // for xmlReaderMethods, XMLReader
-#include "xml_writer.h" // for xmlWriterMethods, XMLWriter
+#include <stdio.h> // for NULL, FILE, perror, fclose, fopen, stdin,
stdout
+#include <string.h> // for strcmp
+#include "daffodil_getopt.h" // for daffodil_cli, parse_daffodil_cli,
daffodil_parse, daffodil_parse_cli, daffodil_unparse, daffodil_unparse_cli,
DAFFODIL_PARSE, DAFFODIL_UNPARSE
Review comment:
No, I don't want to remove the comments. The whole point of the
comments is for me to show a developer looking at the code exactly what is
being used from each include header. If a developer is curious to learn more
about a symbol, he or she can find exactly where to look by searching the
comments to find the right include header and then open that .h file (also the
.c file if both are in the repository).
The includes are not autogenerated. The file has to compile successfully
before iwyu can process it and iwyu doesn't edit the file. It only prints some
replacement includes / comments on standard output along with some other output
and I have to manually edit the file to replace the includes / comments. In
fact, iwyu won't even print the replacement includes / comments if the only
changes would be to the comments (it has no way of knowing what the original
comments were), so I have to manually add an extra include in order for iwyu to
show me the updated comments. I wouldn't bother using iwyu unless it was to
put these comments there as well as get rid of unneeded includes.
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/c/libruntime/errors.c
##########
@@ -17,34 +17,109 @@
#include "errors.h"
#include <assert.h> // for assert
-#include <error.h> // for error
#include <inttypes.h> // for PRId64
#include <stdbool.h> // for bool, false, true
-#include <stdio.h> // for NULL, feof, ferror, FILE, size_t
-#include <stdlib.h> // for EXIT_FAILURE
+#include <stdio.h> // for fprintf, stderr, NULL, feof, ferror, FILE,
size_t, stdout
+#include <stdlib.h> // for exit, EXIT_FAILURE
-// error_message - return an internationalized error message
+// eof_or_error - get pointer to error if stream has eof or error indicator set
+
+const Error *
+eof_or_error(FILE *stream)
+{
+ if (feof(stream))
+ {
+ static Error error = {ERR_STREAM_EOF, {NULL}};
+ return &error;
+ }
+ else if (ferror(stream))
+ {
+ static Error error = {ERR_STREAM_ERROR, {NULL}};
+ return &error;
+ }
+ else
+ {
+ return NULL;
+ }
+}
+
+// get_diagnostics - get pointer to validation diagnostics
+
+Diagnostics *
+get_diagnostics(void)
+{
+ static Diagnostics diagnostics;
+ return &diagnostics;
+}
+
+// add_diagnostic - add a new error to validation diagnostics
+
+bool
+add_diagnostic(Diagnostics *diagnostics, const Error *error)
+{
+ if (diagnostics && error)
+ {
+ if (diagnostics->length < LIMIT_DIAGNOSTICS)
+ {
+ Error *err = &diagnostics->array[diagnostics->length++];
+ err->code = error->code;
+ err->s = error->s;
+ return true;
+ }
+ }
+ return false;
+}
+
+// error_message - get an internationalized error message
static const char *
error_message(enum ErrorCode code)
Review comment:
Sounds like you would prefer that libcli have its own cli_errors.[ch]
with similar enum, struct, and functions just to keep all the CLI_* codes and
messages outside of libruntime. Mike, do you want that too?
##########
File path: .github/workflows/main.yml
##########
@@ -53,7 +59,7 @@ jobs:
if: runner.os == 'Windows'
uses: msys2/setup-msys2@v2
with:
- install: gcc libargp-devel make pkgconf
+ install: clang diffutils make pkgconf
Review comment:
We don't need diffutils to build daffodil in the CI workflow. I was
including it only for consistency with BUILD.md where I wanted to show
developers how to install nearly everything they might need. I'll remove
diffutils to keep the MSYS2 install as small as possible.
##########
File path: BUILD.md
##########
@@ -31,26 +31,32 @@ the latest [SBT] version following their websites'
instructions or
install them using your operating system's package manager.
Since Daffodil now has a C backend as well as a Scala backend, you
-will need a C compiler supporting the [C99] standard or later, the
-[Mini-XML] library, and possibly the GNU [argp] library if your
-system's C library doesn't include it already. You can install either
-[gcc] or [clang] using your operating system's package manager. If
-you can't install the [Mini-XML] library using your operating system's
-package manager, you'll have to build it from source. We'll tell you
-how to do that on Windows but the commands should work on other
-operating systems too.
-
-You can set your environment variables "CC" and "AR" to the correct
+will need a C compiler supporting the [C99] standard or later and the
+[Mini-XML] library. You can install either [gcc] or [clang] using
+your operating system's package manager. If you can't install the
+[Mini-XML] library using your operating system's package manager,
+you'll have to build it from source. We'll tell you how to do that on
+Windows but the commands should work on other operating systems too.
+
+You can set your environment variables `CC` and `AR` to the correct
commands (or set them to `true` to disable C compilation altogether)
if you don't want `sbt compile` to call your C compiler with `cc` and
`ar` as the default commands.
-## Fedora 33
+## Fedora 34
-You can use the `dnf` package manager to install most of the build
-requirements needed to build Daffodil:
+You can use the `dnf` package manager to install most of the tools
+needed to build Daffodil:
- sudo dnf install gcc git java-11-openjdk-devel make mxml-devel pkgconf
+ sudo dnf install clang git java-11-openjdk-devel llvm make mxml-devel
pkgconf
+
+If you want to use clang instead of gcc, you'll have to set your
+environment variables `CC` and `AR` to the clang binaries' names:
+
+ CC=clang
+ AR=llvm-ar
+ export CC
+ export AR
Review comment:
I know there are some differences and incompatibilities between Unix
shells, but I agree it'd be cleaner to do the export in one line (that's what
I've been typing anyway).
##########
File path: BUILD.md
##########
@@ -82,10 +96,18 @@ Install [MSYS2] following its website's instructions and
open a new
"MSYS2 MSYS" window. We'll need its collection of free programs and
libraries.
-Install [gcc] and [libargp][argp] using MSYS2's `pacman` package
-manager:
+You can use the `pacman` package manager to install most of the tools
+needed to build Daffodil:
+
+ pacman -S clang diffutils git make pkgconf
Review comment:
I wanted to show developers how to install nearly everything they might
need for runtime2 development on each platform. The only time you need
diffutils is to run the c/Makefile's "make test" rule, which you'd do only by
hand. Actually you need
[xmldiff](https://xmldiff.readthedocs.io/en/stable/installation.html) for that
Makefile rule too but I decided not to mention xmldiff in BUILD.md because
there's no MSYS2 package and the procedure's too complicated for most
developers (install Python3 and pip, use pip to install xmldiff). I also
thought it would be too complicated to show developers how to build and run
[include-what-you-use](https://github.com/include-what-you-use/include-what-you-use/blob/master/README.md),
another tool that I use for updating the runtime2 C files' includes and
comments showing what is used from each include. Using iwyu requires some
manual editing, so it's not easy to automate in CI either.
##########
File path:
daffodil-runtime2/src/main/resources/org/apache/daffodil/runtime2/examples/NestedUnion.c
##########
@@ -25,7 +25,7 @@
// Initialize our program's name and version
-const char *argp_program_version = "daffodil-runtime2 3.1.0-SNAPSHOT";
+const char *daffodil_program_version = "daffodil-runtime2 3.1.0-SNAPSHOT";
Review comment:
No, this whole file is just a very lightly edited copy of an
autogenerated file to be shown as an example of what generated code looks like.
We've had some discussion about failing the CI workflow if there's a
difference (which would be a hassle) or auto-updating these example files in a
sbt build (which would be better). If we wanted to auto-update these files, we
would have to delete the copyrights and change the filenames (unless you decide
it's fine for the code generator to include the Apache copyright in its
generated_code.[ch] files).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
