Re: Fwd: Update earlier posted hideif.el enhancements

[Stefan Monnier]

> Sure, no problem. Attached is the patch against Emacs trunk.

Thanks.  Came through unadulterated ;-)

> I'm using git://git.savannah.gnu.org/emacs.git as my Emacs remote
> source.  I've checked the trunk, master and emacs-24 and they seems to
> have the identical hideif.el version so I think it should be okay to
> patch it.

Indeed, there's been no real development on it in recently.  See my
comments below.  Overall, I think it looks good and could be included in
hideif.el, but I'd appreciate if you could clean up a few things first
and split the change into a few chunks (this is a bit large to review as
one single chunk).  IIUC one chunk would focus on extend the current
parser & evaluator to handle the full CPP syntax.


        Stefan


>    (if hide-ifdef-mode
>        (progn
>       ;; inherit global values
> -     (set (make-local-variable 'hide-ifdef-env)
> -          (default-value 'hide-ifdef-env))
> +;;   (set (make-local-variable 'hide-ifdef-env) ;; [2012-09-27 11:05:26 
> +0800] Luke rem 2 ins 2
> +;;        (default-value 'hide-ifdef-env))
> +     (set 'hide-ifdef-env (default-value 'hide-ifdef-env))
>       (set (make-local-variable 'hide-ifdef-hiding)
>            (default-value 'hide-ifdef-hiding))
>       (set (make-local-variable 'hif-outside-read-only) buffer-read-only)

I don't undertand this change.  "(set '<foo> ...)" is just a bad form of
(setq <foo> ...) so use `setq' instead.

> +(defun merge-ifdef-region (start end)

This lacks the "hif-" or "hide-ifdef-" prefix.

>  ;;;-----------------------------------------------------------------
>  ;;; Translate C preprocessor #if expressions using recursive descent.
>  ;;; This parser is limited to the operators &&, ||, !, and "defined".
>  ;;; Added ==, !=, +, and -.  Gary Oberbrunner, address@hidden, 8/9/94
> +;;;
> +;;; Implement the C language operator precedence table. Add all those
> +;;; missing operators that could be used in macros. Luke Lee 2013-09-04

";;;" should be limited to headers.  The pre-existing code is already at
fault, but we should fix the pre-existing code rather than repeating
its mistake.

> +;;;  | Unary Operators      | * & + - ! ~ ++expr --expr (typecast) sizeof | 
> right-to-left |

Try really hard to stay within 80 columns, please.

> +(defun hif-expand-token-list (tokens &optional macroname reflist) ;; 
> reference list to prevent self-referencing

Same here.  Also, we like to capitalize our comments and terminate them
with full-stops.

BTW, maybe we could/should add some code to smie.el so as to not only
skip a expression but return a parse tree.  Then we could use maybe it here.

> +(defun shiftleft (a b)
> +(defun shiftright (a b)
> +(defun flatten (l)
> +(defun delimit (l a)

Lack the "hif-" prefix.