On Thu, Dec 14 2023, Ihor Radchenko <yantar92@posteo.net> wrote:

> Leo Butler <Leo.Butler@umanitoba.ca> writes:
>
>> From 7d8e406bc4a92e2e2eab772b2671dcd72ca8c202 Mon Sep 17 00:00:00 2001
>> From: Leo Butler <leo.butler@umanitoba.ca>
>> Date: Tue, 12 Dec 2023 12:32:41 -0600
>> Subject: [PATCH] lisp/ob-C.el: add :compile-only header to compile to a named
>>  target
>
> Thanks for the patch!

Thank you for the feedback.

>
>> * lisp/ob-C.el (org-babel-C-execute): The new header argument,
>> `:compile-only', causes source and compiled binary files to be named
>> using the `:file' header argument.  When `:compile-only' is set,
>> execution of source block ends at compilation.  The naming of source
>> and binary filenames is factored out to `org-babel-C-src/bin-file'.
>
> What will happen if we have something like :results value or :results
> output instead of :results file link?

Originally, I felt that only ":results file" makes sense. I have adopted
your suggestion, though, and added test cases so that the compiler
stderr output is caught.

>
>> * lisp/ob-C.el (org-babel-C-src/bin-file): A new function that factors
>> out the setting of source and binary filenames.  It also signals an
>> error if `:compile-only' is set, but `:file' is not.
>> * testing/examples/ob-C-test.org: Add three example that exercise the
>> `:compile-only' header argument, including one that causes an error.
>> * testing/lisp/test-ob-C.el: Add three tests of the `:compile-only'
>> header argument.  New tests: ob-C/set-src+bin-file-name-{1,2,3}.
>
> You should also announce the new feature in ORG-NEWS and document it in
> WORG.

I have added the announcement in this patch. I will submit a separate
patch for worg.

>
>> +                                    (compile-only . (nil no t yes)))
>
> Why nil/t? No other header argument allow "nil" or "t". Just yes/no.

Ok. I also noticed (in a separate thread) that it should be

(compile-only . ((no yes))

>
>> +(defun org-babel-C-src/bin-file (params src? compile-only?)
>> +  "Return the src or bin filename to `org-babel-C-execute'.
>> +
>> +If `SRC?' is T, a file extension is added to the filename.  By
>
> Just SRC?. You should only quote Elisp symbols and upcase the function
> arguments. See
> https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html
>
> Also, why upcase "T"?

Corrected.

>
>> +default, the filename is created by `org-babel-temp-file'. If
>> +`COMPILE-ONLY?' is T, the filename is taken from the `:file'
>
> I think quoting :file is not necessary.

Yes.

>
>> +field in `PARAMS'; if that is NIL, an error occurs."
>
> No need to upcase NIL.

Yes.

> Also, "if that is nil, throw an error" - this is more common style
> (saying what function does).

Done.

>
>> +  (let ((f (cdr (assq :file params))))
>
> Please avoid short variable names - they are harder to understand and
> search in code.

Done.

>
>> +    (when (and compile-only? (null f))
>> +      (error "Error: When COMPILE-ONLY is T or YES, output FILE needs to be set"))
>
> t or "yes". Also, what does "output FILE" refer to? Upcasing implies
> function argument, but you are referring to :file header argument in PARAMS.

Ok, I think the current error message is consistent with Gnu
standards.

>
>> +    (let* ((file (cond (compile-only? f) (src? "C-src-") (t "C-bin-")))
>> +           (ext (if src? (pcase org-babel-c-variant
>> +	                   (`c ".c") (`cpp ".cpp") (`d ".d"))
>
> We usually split `cond' and `case' into multiple lines for readability.
> Otherwise, it is confusing, especially in `let' forms where one can
> confuse `cond' forms with `let' bindings.

Ok. I guess, to paraphrase, that readability is in the eye of the beholder.

>
>> +    (unless compile-only?
>> +      (let ((results
>> +	     (org-babel-eval
>> ...
>
> No return value at all? I'd expect file link to be returned, as we
> discussed in another thread. Also, see the above considerations about
> :results value/output.

See my comment above.

> We might want return the compiler output for
> :results output.

Done.

> Or maybe even arrange `org-babel-eval-error-notify' to
> display compile-mode window when there are compilation warnings.

Yes, this is already done by `org-babel-eval' in `org-babel-C-execute'.

>
>> --- a/testing/examples/ob-C-test.org
>> +++ b/testing/examples/ob-C-test.org
>> @@ -174,3 +174,29 @@ std::cout << "\"line 1\"\n";
>>  std::cout << "\"line 2\"\n";
>>  std::cout << "\"line 3\"\n";
>>  #+end_src
>
> If you can, please avoid adding tests as Org files where possible.
> Instead, we prefer using `org-test-with-temp-text' or
> `org-test-with-temp-text-in-file' when the Org fragment for the test is
> not too long.

Ok.

Patch attached.

Leo