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.

Sign up for GitHub

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

Add support for writing to ERROR_OUTPUT from kernel code #3043

Merged
merged 3 commits into from
Nov 29, 2018
Merged

Add support for writing to ERROR_OUTPUT from kernel code #3043

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd61c7f
kernel: add helper function for writing error messages to the
embray Nov 22, 2018
3eb2f2a
fix objectionable assignment expression
embray Nov 23, 2018
2949b24
explicitly check if ERROR_OUTPUT is a stream object before attempting…
embray Nov 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,49 @@


static Obj ErrorInner;
static Obj ERROR_OUTPUT = NULL;


/****************************************************************************
**
*F * * * * * * * * * * * * * * error functions * * * * * * * * * * * * * * *
*/

/****************************************************************************
**
*F OpenErrorOutput() . . . . . . . open the file or stream assigned to the
** ERROR_OUTPUT global variable defined in
** error.g, or "*errout*" otherwise
*/
UInt OpenErrorOutput( void )
{
/* Try to print the output to stream. Use *errout* as a fallback. */
UInt ret = 0;

if (ERROR_OUTPUT != NULL) {
if (IsStringConv(ERROR_OUTPUT)) {
ret = OpenOutput(CONST_CSTR_STRING(ERROR_OUTPUT));
}
else {
ret = OpenOutputStream(ERROR_OUTPUT);
Copy link
Member

Choose a reason for hiding this comment

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

We started to add more validation, and here, one could verify that ERROR_OUTPUT actually is a stream; see e.g. PRINT_OR_APPEND_TO_FILE_OR_STREAM.

On that note, perhaps it would be "better" to put this new function into streams.c or io.c...; but I have no strong conviction in either direction, and in any case, it's not hard to move a function to another file later on, should we feel the need for it :-).

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 wanted to check that it was actually a stream, but I couldn't quickly find the "right" way to check that. Is there a TNUM for stream objects? I'll look at those functions you mentioned.

I agree this function might make more sense in a io.c. I put it in error.c because that was already loading another global variable from error.g. I don't know if that entirely matters though: The connection between the C-based modules and the GAP language library modules is one that I haven't deeply explored yet and is unclear to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, TNUMs only cover a very small, hard-wired list of types with special representations in the GAP kernel (things like integers, booleans, permutations). For things like streams, you want to use the more general IsOutputStream. As Max mentioned, you can see how to get this GAP level variable, and copy a reference to it into C, in PRINT_OR_APPEND_TO_FILE_OR_STREAM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, clearly I did not even test my last commit. I'm just going to delete it.

}
}

if (!ret) {
/* It may be we already tried and failed to open *errout* above but
* but this is an extreme case so it can't hurt to try again
* anyways */
if ((ret = OpenOutput("*errout*"))) {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't assign inside the if, i.e., first assign, then test.

Pr("failed to open error stream\n", 0, 0);
}
else {
Panic("failed to open *errout*");
}
}

return ret;
}


/****************************************************************************
**
Expand Down Expand Up @@ -581,6 +617,7 @@ static Int InitKernel(StructInitInfo * module)
InitHdlrFuncsFromTable(GVarFuncs);

ImportFuncFromLibrary("ErrorInner", &ErrorInner);
ImportGVarFromLibrary("ERROR_OUTPUT", &ERROR_OUTPUT);

// return success
return 0;
Expand Down
8 changes: 8 additions & 0 deletions src/error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ typedef void (*intfunc)(Int);

Int RegisterBreakloopObserver(intfunc func);

/****************************************************************************
**
*F OpenErrorOutput() . . . . . . . open the file or stream assigned to the
** ERROR_OUTPUT global variable defined in
** error.g, or "*errout*" otherwise
*/
extern UInt OpenErrorOutput();

/****************************************************************************
**
*F ErrorQuit( <msg>, <arg1>, <arg2> ) . . . . . . . . . . . print and quit
Expand Down
3 changes: 2 additions & 1 deletion src/scanner.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "scanner.h"

#include "error.h"
#include "gapstate.h"
#include "gaputils.h"
#include "io.h"
Expand All @@ -42,7 +43,7 @@ static void SyntaxErrorOrWarning(const Char * msg, UInt error)
if (STATE(NrErrLine) == 0) {

// open error output
OpenOutput("*errout*");
OpenErrorOutput();

// print the message ...
if (error)
Expand Down