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 20 forks source link

Small fixes from profiling PR #126

Closed lefessan closed 6 months ago

lefessan commented 8 months ago

Do not move object files if they were specified as an explicit target on the command line (typically cobc -c --save-temps=DIR -o foo.o foo.cob should keep foo.o in the current directory)

codecov-commenter commented 8 months ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (c0d64ad) 65.74% compared to head (a07514b) 65.76%.

Files Patch % Lines
cobc/cobc.c 25.00% 0 Missing and 3 partials :warning:
libcob/common.c 25.00% 2 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## gcos4gnucobol-3.x #126 +/- ## ===================================================== + Coverage 65.74% 65.76% +0.02% ===================================================== Files 32 32 Lines 59092 59095 +3 Branches 15575 15577 +2 ===================================================== + Hits 38849 38865 +16 + Misses 14262 14251 -11 + Partials 5981 5979 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GitMensch commented 8 months ago

Yes, because this generated file has no inclusion guard - it should be included only once - and should be included before any system header in any case. As tarstamp.h may be used in there (that was kind of common for environments without auto-configuration in the patchlevel, but we could drop support for that) it is included before config.h

lefessan commented 8 months ago

Yes, config.h is generated, but some of the defined macros must be defined, so we could have:

#ifndef COBCRUN_NAME
#include "config.h"
#endif 

?

GitMensch commented 8 months ago

In the current code that would be

#ifndef COBCRUN_NAME
#ifndef COB_TAR_DATE
#include "tarstamp.h"
#endif
#include "config.h"
#endif

in both cobc/cobc.h and libcob/coblocal.h

I'd prefer the more easy one to just have

/* may be used in config.h, e.g. for PATCH_LEVEL */
#include "tarstamp.h"
/* (generated) configuration for this build */
#include "config.h"

in all source files as the first code line in the beginning.

... thinking of this again, tarstamp.h may only be used in config.h if it is manually created (like in build_windows/config.h.in), so we should just include it there and in the places using it (without checking: libcob/common.c, cobc/cobc.c, cobc/codegen.c); I'll do this change to the 3.x branch "soon".

Further: actually config.h should only be used if HAVE_CONFIG_H is defined "to be clean" but I think it isn't reasonable to expect anyone to pass that much defines on the command line, so plan to ignore this.

lefessan commented 8 months ago

I'd prefer the more easy one to just have in all source files as the first code line in the beginning.

I am not fond of such "informal convention", that may lead to bugs the next time somebody creates a new source file. Just during this PR, I had to debug weird behaviors twice, because of the position of config.h before or after other include files.

Another idea: create a header file cob-config.h that contains (or whatever code needed to load the config):

#ifndef COB_CONFIG_H
#ifndef COBCRUN_NAME
#ifndef COB_TAR_DATE
#include "tarstamp.h"
#endif
#include "config.h"
#endif
#endif

and have it included from all other include files, and replace config.h in source files.

GitMensch commented 8 months ago

... but even then this cob-config.h must either be included everywhere or included in coblocal.h and cobc.h, no? In this case it would be better placed into those files directly.

Note that 4.x has a new "private" compile-time header, we can include something like that there in any case.

lefessan commented 7 months ago

I simplified this PR to only fix the --save-temps=DIR behavior, and export cob_get_strerror in coblocal.h.

For the config.h, I am not sure that adding the check in cobc.h would be ok, as this file is usually included only very late in files, while config.h must be the first included files (even before system includes).

I was also surprised that the configuration is not exported during installation, I would have expected libcob/common.h to include config.h and have config.h found there too (it might be useful to check the config.h after installation to see what flags where used).

Anyway, I propose to have this discussion in another PR, maybe for 4.x, when we start targetting it for next changes. By the way, is there a thread somewhere on how the switch between 3.x to 4.x is organized ?