Leo Butler <leo.but...@umanitoba.ca> writes: > From 7d8e406bc4a92e2e2eab772b2671dcd72ca8c202 Mon Sep 17 00:00:00 2001 > From: Leo Butler <leo.but...@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! > * 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? > * 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. > + (compile-only . (nil no t yes))) Why nil/t? No other header argument allow "nil" or "t". Just yes/no. > +(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"? > +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. > +field in `PARAMS'; if that is NIL, an error occurs." No need to upcase NIL. Also, "if that is nil, throw an error" - this is more common style (saying what function does). > + (let ((f (cdr (assq :file params)))) Please avoid short variable names - they are harder to understand and search in code. > + (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. > + (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. > + (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. We might want return the compiler output for :results output. Or maybe even arrange `org-babel-eval-error-notify' to display compile-mode window when there are compilation warnings. > --- 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. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>