Skip to content

Conversation

@sfkleach
Copy link
Member

@sfkleach sfkleach commented Sep 4, 2022

This adds a new flag '-quiet' to the popc compiler that suppresses the garbage collection message and also the printing of file names to stderr.

@sfkleach sfkleach changed the title Feature/popc quiet flag Feature/popc quiet flag, closing #126 Sep 4, 2022
@sfkleach sfkleach requested a review from willprice September 4, 2022 10:16
@sfkleach sfkleach self-assigned this Sep 4, 2022
@sfkleach sfkleach changed the title Feature/popc quiet flag, closing #126 DRAFT: Feature/popc quiet flag, closing #126 Sep 4, 2022
@sfkleach sfkleach changed the title DRAFT: Feature/popc quiet flag, closing #126 DRAFT - Feature/popc quiet flag, closing #126 Sep 4, 2022
@sfkleach sfkleach marked this pull request as draft September 4, 2022 10:56
@sfkleach sfkleach changed the title DRAFT - Feature/popc quiet flag, closing #126 Feature/popc quiet flag, closing #126 Sep 4, 2022
@sfkleach sfkleach force-pushed the feature/popc_quiet_flag branch from cb1eec5 to 8bafb88 Compare September 5, 2022 07:24
@sfkleach sfkleach marked this pull request as ready for review September 5, 2022 07:34
@sfkleach
Copy link
Member Author

sfkleach commented Sep 15, 2022

Waldek made comments as follows:

I must admit that I have doubts about this patch. Namely, there
is already a flag, namely 'l_flag' that controls printing of
files. Extra machinery, that is 'cucharverbose' to control
the same thing looks undesirable (unnecessary complexity).
I think it would be better to to set 'l_flag' to false to
get the effect.

Of course, there is a difficulty: option processing may set
'l_flag' to true after '-quiet' set it to false. One solution,
which is simple is to set 'l_flag' only due to explicit request
(that is '-l' option). Another is to change '-quiet' handling to

                elseif c == "quiet" then
                    false -> popgctrace;
                    "quiet" -> l_flag;

and assign 'l_flag or true' to 'l_flag' during option processing.

And at end add

   if l_flag == "quiet" then false -> l_flag endif;

Or, set extra variable during processing of 'quiet' and if that
variable is set assign 'false' to 'l_flag'.

Concerning build: I agree that by default it should less chatty,
but IMO this should be controlled by variable, so that one
can turn on verbosity from command line.

BTW: For build I usually use commands like

make > mlogg 2>&1

If everything goes OK I can discard 'mlogg' later. In case
of troubles verbosity may help...

@sfkleach
Copy link
Member Author

I agree that using the preexisting l_flag is a less intrusive change and I have adopted Waldek's suggestion almost in its entirety. The suggestion that the verbosity should be controlled by a variable I have not followed through for these reasons.

  • It is unclear whether the intention is an environment variable or a pop11 variable (e.g. some variant on pop_debugging) .
  • If there is a -l flag to generate a listing then it makes sense for there to be a -q flag to inhibit the listing. To my way of thinking it is a pretty clear omission in the design of the option flags.
  • It should be easy enough to incorporate this suggestion when it is clarified.

@hebisch
Copy link
Collaborator

hebisch commented Sep 20, 2022

First to clarify, I wrote:

: Concerning build: I agree that by default it should less chatty,
: but IMO this should be controlled by variable, so that one
: can turn on verbosity from command line.

This is not about popc. Rather I mean Makefiles for Poplog build.
And by variable I mean make variable.

Concerning patches, I have two remarks:

  • As is last of -q and -l wins. I am not sure if this is useful. Code would be
    simpler if -q always wins. This is minor issue, I mention this to make sure
    that you really want this.
  • In my code I am trying to keep line length below 80 characters. Your
    comments creates longer lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants