From: João Távora Date: Thu, 28 Sep 2017 11:06:56 +0000 (+0100) Subject: Fix three Flymake bugs when checking C header files X-Git-Tag: emacs-26.0.90~56^2^2~14 X-Git-Url: http://git.eshelyaron.com/gitweb/?a=commitdiff_plain;h=8118f0f95f993f64f30ab1d48d9e988ab6f58019;p=emacs.git Fix three Flymake bugs when checking C header files The first of these problems is longstanding: if an error-less B.h is included from error-ridden A.h, flymake's legacy parser will panic (and disable itself) since it sees a non-zero exit for a clean file. To fix this, recommend returning 'true' in the documentation for the check-syntax target. Another problem was introduced by the parser rewrite. For error patterns spanning more than one line, point may be left in the middle of a line and thus render other patterns useless. Those patterns were written for the old line-by-line parser. To make them useful again, move to the beginning of line in those situations. The third problem was also longstanding and happened on newer GCC's: The "In file included from" prefix confused flymake-proc-get-real-file-name. Fix this. Also updated flymake--diag-region to fallback to highlighting a full line less often. Add automatic tests to check this. * lisp/progmodes/flymake-proc.el (flymake-proc--diagnostics-for-pattern): Fix bug when patterns accidentally spans more than one line. Don't create diagnostics without error messages. (flymake-proc-real-file-name-considering-includes): New helper. (flymake-proc-allowed-file-name-masks): Use it. * lisp/progmodes/flymake.el (flymake-diag-region): Make COL argument explicitly optional. Only fall back to full line in extreme cases. * test/lisp/progmodes/flymake-tests.el (included-c-header-files): New test. (different-diagnostic-types): Update. * test/lisp/progmodes/flymake-resources/Makefile (check-syntax): Always return success (0) error code. (CC_OPTS): Add -Wextra * test/lisp/progmodes/flymake-resources/errors-and-warnings.c (main): Rewrite comments. * test/lisp/progmodes/flymake-resources/errors-and-warnings.c: Include some dummy header files. * test/lisp/progmodes/flymake-resources/no-problems.h: New file. * test/lisp/progmodes/flymake-resources/some-problems.h: New file. * doc/misc/flymake.texi (Example---Configuring a tool called via make): Recommend adding "|| true" to the check-syntax target. --- diff --git a/doc/misc/flymake.texi b/doc/misc/flymake.texi index 1bc416fd02e..01849b7d9a5 100644 --- a/doc/misc/flymake.texi +++ b/doc/misc/flymake.texi @@ -492,7 +492,7 @@ our case this target might look like this: @verbatim check-syntax: - gcc -o /dev/null -S ${CHK_SOURCES} + gcc -o /dev/null -S ${CHK_SOURCES} || true @end verbatim @noindent @@ -504,7 +504,7 @@ Automake variable @code{COMPILE}: @verbatim check-syntax: - $(COMPILE) -o /dev/null -S ${CHK_SOURCES} + $(COMPILE) -o /dev/null -S ${CHK_SOURCES} || true @end verbatim @node Flymake Implementation diff --git a/lisp/progmodes/flymake-proc.el b/lisp/progmodes/flymake-proc.el index 37b7e49dea4..48d35598b72 100644 --- a/lisp/progmodes/flymake-proc.el +++ b/lisp/progmodes/flymake-proc.el @@ -66,7 +66,10 @@ :type 'integer) (defcustom flymake-proc-allowed-file-name-masks - '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'" flymake-proc-simple-make-init) + '(("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'" + flymake-proc-simple-make-init + nil + flymake-proc-real-file-name-considering-includes) ("\\.xml\\'" flymake-proc-xml-init) ("\\.html?\\'" flymake-proc-xml-init) ("\\.cs\\'" flymake-proc-simple-make-init) @@ -419,12 +422,25 @@ Create parent directories as needed." (condition-case-unless-debug err (cl-loop with (regexp file-idx line-idx col-idx message-idx) = pattern - while (search-forward-regexp regexp nil t) + while (and + (search-forward-regexp regexp nil t) + ;; If the preceding search spanned more than one line, + ;; move to the start of the line we ended up in. This + ;; preserves the usefulness of the patterns in + ;; `flymake-proc-err-line-patterns', which were + ;; written primarily for flymake's original + ;; line-by-line parsing and thus never spanned + ;; multiple lines. + (if (/= (line-number-at-pos (match-beginning 0)) + (line-number-at-pos)) + (goto-char (line-beginning-position)) + t)) for fname = (and file-idx (match-string file-idx)) for message = (and message-idx (match-string message-idx)) for line-string = (and line-idx (match-string line-idx)) - for line-number = (and line-string - (string-to-number line-string)) + for line-number = (or (and line-string + (string-to-number line-string)) + 1) for col-string = (and col-idx (match-string col-idx)) for col-number = (and col-string (string-to-number col-string)) @@ -436,7 +452,7 @@ Create parent directories as needed." fname))) for buffer = (and full-file (find-buffer-visiting full-file)) - if (eq buffer (process-buffer proc)) + if (and (eq buffer (process-buffer proc)) message) collect (with-current-buffer buffer (pcase-let ((`(,beg . ,end) (flymake-diag-region line-number col-number))) @@ -1030,6 +1046,13 @@ Use CREATE-TEMP-F for creating temp copy." '("\\.\\(?:c\\(?:pp\\|xx\\|\\+\\+\\)?\\|CC\\)\\'") "[ \t]*#[ \t]*include[ \t]*\"\\([[:word:]0-9/\\_.]*%s\\)\"")) +(defun flymake-proc-real-file-name-considering-includes (scraped) + (flymake-proc-get-real-file-name + (let ((case-fold-search t)) + (replace-regexp-in-string "^in file included from[ \t*]" + "" + scraped)))) + ;;;; .java/make specific (defun flymake-proc-simple-make-java-init () (flymake-proc-simple-make-init-impl 'flymake-proc-create-temp-with-folder-structure nil nil "Makefile" 'flymake-proc-get-make-cmdline)) diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el index 282727e315d..285ef93db62 100644 --- a/lisp/progmodes/flymake.el +++ b/lisp/progmodes/flymake.el @@ -248,9 +248,10 @@ verify FILTER, sort them by COMPARE (using KEY)." (define-obsolete-face-alias 'flymake-warnline 'flymake-warning "26.1") (define-obsolete-face-alias 'flymake-errline 'flymake-error "26.1") -(defun flymake-diag-region (line col) +(defun flymake-diag-region (line &optional col) "Compute region (BEG . END) corresponding to LINE and COL. -Or nil if the region is invalid." +If COL is nil, return a region just for LINE. +Return nil if the region is invalid." (condition-case-unless-debug _err (let ((line (min (max line 1) (line-number-at-pos (point-max) 'absolute)))) @@ -267,13 +268,18 @@ Or nil if the region is invalid." (if (eq (point) beg) (line-beginning-position 2) (point))))) - (if col - (let* ((beg (progn (forward-char (1- col)) (point))) + (if (and col (cl-plusp col)) + (let* ((beg (progn (forward-char (1- col)) + (point))) (sexp-end (ignore-errors (end-of-thing 'sexp))) - (end (or sexp-end - (fallback-eol beg)))) - (cons (if sexp-end beg (fallback-bol)) - end)) + (end (or (and sexp-end + (not (= sexp-end beg)) + sexp-end) + (ignore-errors (goto-char (1+ beg))))) + (safe-end (or end + (fallback-eol beg)))) + (cons (if end beg (fallback-bol)) + safe-end)) (let* ((beg (fallback-bol)) (end (fallback-eol beg))) (cons beg end)))))) diff --git a/test/lisp/progmodes/flymake-resources/Makefile b/test/lisp/progmodes/flymake-resources/Makefile index 0f3f39791c8..494407567f2 100644 --- a/test/lisp/progmodes/flymake-resources/Makefile +++ b/test/lisp/progmodes/flymake-resources/Makefile @@ -1,6 +1,6 @@ # Makefile for flymake tests -CC_OPTS = -Wall +CC_OPTS = -Wall -Wextra ## Recent gcc (e.g. 4.8.2 on RHEL7) can automatically colorize their output, ## which can confuse flymake. Set GCC_COLORS to disable that. @@ -8,6 +8,6 @@ CC_OPTS = -Wall ## normally use flymake, so it seems like just avoiding the issue ## in this test is fine. Set flymake-log-level to 3 to investigate. check-syntax: - GCC_COLORS= $(CC) $(CC_OPTS) ${CHK_SOURCES} + GCC_COLORS= $(CC) $(CC_OPTS) ${CHK_SOURCES} || true # eof diff --git a/test/lisp/progmodes/flymake-resources/errors-and-warnings.c b/test/lisp/progmodes/flymake-resources/errors-and-warnings.c index 6454dd20236..1d38bd6bd27 100644 --- a/test/lisp/progmodes/flymake-resources/errors-and-warnings.c +++ b/test/lisp/progmodes/flymake-resources/errors-and-warnings.c @@ -1,10 +1,13 @@ - int main() +/* Flymake should notice an error on the next line, since + that file has at least one warning.*/ +#include "some-problems.h" +/* But not this one */ +#include "no-problems.h" + +int main() { - char c = 1000; + char c = 1000; /* a note and a warning */ int bla; - /* The following line should have one warning and one error. The - warning spans the full line because gcc (at least 6.3.0) points - places the error at the =, which isn't a sexp.*/ - char c; if (bla == (void*)3); + char c; if (bla == (void*)3); /* an error, and two warnings */ return c; } diff --git a/test/lisp/progmodes/flymake-resources/no-problems.h b/test/lisp/progmodes/flymake-resources/no-problems.h new file mode 100644 index 00000000000..19ddc615b32 --- /dev/null +++ b/test/lisp/progmodes/flymake-resources/no-problems.h @@ -0,0 +1 @@ +typedef int no_problems; diff --git a/test/lisp/progmodes/flymake-resources/some-problems.h b/test/lisp/progmodes/flymake-resources/some-problems.h new file mode 100644 index 00000000000..165d8dd525e --- /dev/null +++ b/test/lisp/progmodes/flymake-resources/some-problems.h @@ -0,0 +1,5 @@ +#include + +strange; + +sint main(); diff --git a/test/lisp/progmodes/flymake-tests.el b/test/lisp/progmodes/flymake-tests.el index fa77a9a8ae6..222c8f11848 100644 --- a/test/lisp/progmodes/flymake-tests.el +++ b/test/lisp/progmodes/flymake-tests.el @@ -122,13 +122,33 @@ SEVERITY-PREDICATE is used to setup (flymake-tests--with-flymake ("errors-and-warnings.c") (flymake-goto-next-error) + (should (eq 'flymake-error (face-at-point))) + (flymake-goto-next-error) (should (eq 'flymake-note (face-at-point))) (flymake-goto-next-error) (should (eq 'flymake-warning (face-at-point))) (flymake-goto-next-error) + (should (eq 'flymake-error (face-at-point))) + (flymake-goto-next-error) + (should (eq 'flymake-warning (face-at-point))) + (flymake-goto-next-error) + (should (eq 'flymake-warning (face-at-point))) + (let ((flymake-wrap-around nil)) + (should-error (flymake-goto-next-error nil nil t))) )) + +(ert-deftest included-c-header-files () + "Test inclusion of .h header files." + (skip-unless (and (executable-find "gcc") (executable-find "make"))) + (flymake-tests--with-flymake + ("some-problems.h") + (flymake-goto-next-error) (should (eq 'flymake-warning (face-at-point))) (flymake-goto-next-error) (should (eq 'flymake-error (face-at-point))) + (let ((flymake-wrap-around nil)) + (should-error (flymake-goto-next-error nil nil t))) ) + (flymake-tests--with-flymake + ("no-problems.h") (let ((flymake-wrap-around nil)) (should-error (flymake-goto-next-error nil nil t))) ))