search

OCamlPro / gnucobol

A clone of the sourceforge GnuCOBOL compiler from COBOL to C.
https://get-superbol.com
GNU Lesser General Public License v3.0
16 stars 21 forks source link

Add -fdiagnostics-show-caret to display the context of an error #85

Closed lefessan closed 1 year ago

lefessan commented 1 year ago

When a warning or error is printed, prints the source context with the line of the error, plus 2 lines before and after. COBC_PREVIEW_ERROR can be used to set the option globally.

lefessan commented 1 year ago

As an example:

prompt> $COMPILE -j -fpreview-error prog.cob
prog.cob:6: error: syntax error, unexpected Identifier

  0004          PROCEDURE        DIVISION.
  0005              DISPLAY "jobby"
  0006 >            EXIT HERE.
lefessan commented 1 year ago

Note that the default is currently to disable this feature, but it could be activated by default (and disabled through COBC_PREVIEW_ERROR=0 for the testsuites)

GitMensch commented 1 year ago

Hm, the idea is good and the general point of reworking the diagnostic format is even on the GSOC idea list for GnuCOBOL (did you get it from there/the note on the dev list about that?).

But reading the source file again should be absolute last resort and will be very likely wrong for REPLACE/REPLACING and won't work for complies from stdin.

So... Could you please rework that to possibly save the context from the scanner via to a static buffer?

Also note that -fno-diagnostics-show-line-numbers should be added along with this change (which would drop the leading line numbers) and possibly -fdiagnostics-show-caret should be used to toggle this instead of -fpreview-error ... and ideally we should also use the caret to show where the error is.

As a general rule of thumb we'd like to reach https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Message-Formatting-Options.html ... someday.

lefessan commented 1 year ago

Hm, the idea is good and the general point of reworking the diagnostic format is even on the GSOC idea list for GnuCOBOL (did you get it from there/the note on the dev list about that?).

No, I didn't know about the GSOC idea. Is it discussed on the forum ? I did that patch because I needed it myself... which is usually how I work, as I can only devote a small amount of time on the project from time to time.

But reading the source file again should be absolute last resort and will be very likely wrong for REPLACE/REPLACING and won't work for complies from stdin. So... Could you please rework that to possibly save the context from the scanner via to a static buffer? Also note that -fno-diagnostics-show-line-numbers should be added along with this change (which would drop the leading line numbers) and possibly -fdiagnostics-show-caret should be used to toggle this instead of -fpreview-error ... and ideally we should also use the caret to show where the error is.

I won't be able to spend much more time on this patch. For now, I changed the name of the option to -fdiagnostics-show-caret and added another option to disable it for warnings (typically, we are working on a project where there are currently a lot of warnings, and we want to only focus on errors in a first step).

As a general rule of thumb we'd like to reach https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Message-Formatting-Options.html ... someday.

Would it be ok to merge this work as it is, and wait for the GSOC to get it improved ? I will be working on adding a bit more GCOS support now, so I won't be able to improve this patch more before some time.

lefessan commented 1 year ago

@GitMensch Hi, I say on the forum that you mentioned this patch. Do you think it would be ok to merge it in the current state ?

GitMensch commented 1 year ago

friendly ping @lefessan

lefessan commented 1 year ago

I think all your requests have been fixed.

GitMensch commented 1 year ago

Possibly use in the NEWS something similar to GCCs entry:

the diagnostics now print the source code context with a left margin showing line numbers, configurable with -fno-diagnostics-show-line-numbers, and possible to disable completely with -fno-diagnostics-show-caret;

the option -fdiagnostics-plain-output was added to request that diagnostic output look as plain as possible and stay more stable over time

lefessan commented 1 year ago

I think I added all your suggestions, I will commit it to svn as soon as CI is passed. Thanks !

lefessan commented 1 year ago

Merged in SVN

GitMensch commented 1 year ago

Sadly that broke C89 support in cobc/error.c: https://ci.appveyor.com/api/buildjobs/vqa49l488as7peee/log

Please adjust to declare the variables at the beginning of the scope.

Apart from that: what do you think of adding a std=c89 to the CI here? That should have likely called that.

lefessan commented 1 year ago

Sorry, I didn't see your comment, I am going to fix this problem asap

GitMensch commented 1 year ago

Please drop a note if you think you'll have moved around the declarations in the next two hours as I'm doing it otherwise afterwards.

GitMensch commented 1 year ago

Fixing C89 and additional changes to the format and some of the details done "upstream" in svn r5081.

CI PR already referenced.

GitMensch commented 1 year ago

Can you have a look for some late-cleanup on this, moving -fdiagnostics-plain-output from COBC to COBOL_FLAGS, please? I'd like to keep the command line for NIST, which uses COBC, as small as possible.

--> Note: I'm working on this now.

GitMensch commented 1 year ago

In the meantime I've did that adjustment upstream.