stevedlawrence commented on a change in pull request #545:
URL: https://github.com/apache/daffodil/pull/545#discussion_r623987273
##########
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:
Is diffutils needed?
##########
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:
Should we suggest doing these on one line, e.g. ``export CC=clang`` to
make things cleaner. Or are there cases where that doesn't work?
##########
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:
Same question about diffutils here?
##########
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:
What about just removing those comments? If the includes are
autogenerated, is the information about why they are added useful? Seems it's
only useful if you don't trust the autogenerated includes and want to verify
why it added lines, which doesn't seem that necessary.
##########
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:
Is this hardcoded version going to cause tests to fail when we update
the version?
##########
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:
To me, feels like anything CLI related should not be in libruntime. The
CLI is just a user of libruntime, and just like any other runtime user, they
shouldn't need to update the runtime to change user specific error messages.
Almost feels like the CLI needs a completely separate error path, or libruntime
needs someway for users to inject addition error capabilities.
--
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]
