-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gcov: add support for CMake-based projects and UI enhancement #53
base: master
Are you sure you want to change the base?
Conversation
For CMake projects, the `.gcov` files aren't stored beside the source file, instead, they are placed next to the generated object files (in CMake projects, it will be somewhere like `path/to/build/dir/CMakeFiles/target.src/path/to/file/file.cpp.gcov`) Also, as CMake generates object files as `file.cpp.o` instead of `file.o`, gcov generates a file based on that `file.cpp.gcov` and not `file.gcov`. I've added a function which leverages the information included in the `compile_commands.json`, it extracts the build directory and build command for the file, and then constructs the path to the `.gcov` file which will be next to the `.o` file.
cov.el
Outdated
(when-let* ((root (project-root (project-current))) | ||
(compile-commands (expand-file-name "compile_commands.json" root))) | ||
(when (file-exists-p compile-commands) | ||
(when-let* ((file-cmd (cl-find-if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when-let*
was introduced in Emacs 26 (as far as I can tell). cov.el supports Emacs 24.4 onward. Please use other means to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, I fixed this!
cov.el
Outdated
(when (file-exists-p compile-commands) | ||
(when-let* ((file-cmd (cl-find-if | ||
(lambda (entry) | ||
(equal (file-truename (alist-get 'file entry)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alist-get
was introduced in Emacs 25.1, cov.el supports from Emacs 24.4 onward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, I fixed this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is about the cmake-gcov commit.
I know very little about CMake and compile_commands.json, but the solution seems fragile.
- I think compile_commands.json is optional for CMake?
- Other systems also use compile_commands.json, and this might confuse cov.el in those cases. Or possible work better. I'm not sure.
There are no tests included in this commit, which I think is a must. It would be great if you could supply a test project like in test/lcov
so we can see what the actual files look like. Also check if any of the docs in the README.md files needs to be updated.
Using something like compile_commands.json to locate the output files for build systems that splits source and object directories seems like a good thing. Is there no other information in the file that we can use?
I think this could be handled by correct values in cov-coverage-file-paths
and possibly some other variable, but I can appreciate that this would probably require manual setup in every checkout of the project and that is not the most user friendly way of doing things.
cov.el
Outdated
(command (alist-get 'command file-cmd)) | ||
(directory (alist-get 'directory file-cmd))) | ||
(let* ((obj-file (when (string-match | ||
(concat "-o \\(?1:.*" (regexp-quote (concat file-name ".o")) "\\)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This match looks fragile to me. What if there is no space between -o and the file name for instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted, I've changed it to " -o *"
..., while accepting matches "file.cpp.o"
and "file.o"
for a "file.cpp"
. This is more generic than assuming object files will have the "file.cpp.o"
format (which is correct for CMake, but not necessary valid for other build tools).
I think it would have made more sense to submit these as two separate PRs, but please do not close this PR if you decide to split it. |
cov.el
Outdated
@@ -89,6 +90,16 @@ See `fringe-bitmaps' for a full list of options" | |||
:group 'cov | |||
:type 'symbol) | |||
|
|||
(defcustom cov-highlight-lines nil | |||
"Hightlight lines." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering this is a customization option I think it should have much better documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right!, I was a bit too exited to test the new overlays!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The highlight-line change needs tests and documentation.
a1fed23
to
b3447f9
Compare
b3447f9
to
f9b4c7b
Compare
Hello @snogge , Thank you for the reviews, I've fixed some of your spotted issues, mainly for the CMake+gcov stuff. I will try to fix documentation and tests issues later when I have some free time. I will leave this PR for CMake+gcov stuff. So reverted the highlighting commit, I will open a separate PR for that. |
This reverts commit 44c2f0b.
To respond to your comments, for the The file is generally generated in the build directory, so when configuring a CMake project with a command like this:
The So I think it is Ok to assume the file is in the root directory, but if it is not, the I've added a last check in the function to assure the generated I will try later to add a toy example for a CMake based project to test the code, as well as mentioning this in the README file. |
(let* ((case-fold-search nil) | ||
(obj-file (when (string-match | ||
(concat | ||
" -o[ ]*\\(?1:.*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want \\s-*
instead of [ ]*
;; The buffer is _not_ automatically widened. It is possible to | ||
;; read just a portion of the buffer by narrowing it first. | ||
(let ((line-re (rx line-start | ||
;; note the group numbers are in reverse order | ||
;; in the first alternative | ||
(or (seq (* blank) (group-n 2 (+ (in digit ?#))) ?: | ||
(* blank) (group-n 1 (+ digit)) ?:) | ||
(seq "lcount:" (group-n 1 (+ digit)) ?, (group-n 2 (+ digit)))))) | ||
;; Derive the name of the covered file from the filename of | ||
;; the coverage file. | ||
(filename (file-name-sans-extension (f-filename cov-coverage-file)))) | ||
;; The buffer is _not_ automatically widened. It is possible to | ||
;; read just a portion of the buffer by narrowing it first. | ||
(let ((line-re (rx line-start | ||
;; note the group numbers are in reverse order | ||
;; in the first alternative | ||
(or (seq (* blank) (group-n 2 (+ (in digit ?#))) ?: | ||
(* blank) (group-n 1 (+ digit)) ?:) | ||
(seq "lcount:" (group-n 1 (+ digit)) ?, (group-n 2 (+ digit)))))) | ||
;; Derive the name of the covered file from the filename of | ||
;; the coverage file. | ||
(filename (file-name-sans-extension (f-filename cov-coverage-file)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid whitespace only changes.
(save-excursion | ||
(save-match-data | ||
(goto-char (point-min)) | ||
(list (cons filename | ||
(cl-loop | ||
while (re-search-forward line-re nil t) | ||
collect (list (string-to-number (match-string 1)) | ||
(string-to-number (match-string 2))))))))))) | ||
(save-excursion | ||
(save-match-data | ||
(goto-char (point-min)) | ||
(list (cons filename | ||
(cl-loop | ||
while (re-search-forward line-re nil t) | ||
collect (list (string-to-number (match-string 1)) | ||
(string-to-number (match-string 2))))))))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid whitespace only changes.
For CMake projects, the
.gcov
files aren't stored beside the source file, instead, they are placed next to the generated object files (in CMake projects, it will be somewhere likepath/to/build/dir/CMakeFiles/target.src/path/to/file/file.cpp.gcov
)Also, as CMake generates object files as
file.cpp.o
instead offile.o
, gcov generates a file based on thatfile.cpp.gcov
and notfile.gcov
.I've added a function which leverages the information included in the
compile_commands.json
, it extracts the build directory and the build command for the file, and then constructs the path to the.gcov
file which will be next to the.o
file.The second commit adds support for line highlighting with bright versions of
cov-*-face
s. This feature is configurable viacov-highlight-lines
, I've set its initial value tonil
.Here are some examples:
cov-highlight-lines = t, cov-coverage-mode = t
cov-highlight-lines = t, cov-coverage-mode = nil