bug#17854: The patch #3 of 3 for hideif.el, a lot of bug fixes and enhancements

[Glenn Morris]

> with white space changes removed (git diff -w) as attached.

I hope that the version you install will actually not change the
whitespace, rather than that just being hidden with diff -w.

>  (defcustom hide-ifdef-exclude-define-regexp nil
>    "Ignore #define names if those names match this exclusion pattern."
> -  :type 'string)
> +  :type 'string
> +  :version "24.4")

Why is the :version changing?
In any case, it is the wrong :version, since this won't be in 24.4.


> +(defcustom hide-ifdef-expand-reinclusion-protection t
> +  "Prevent hiding the whole C/C++ header file protected by a big 
> #ifdef..#endif.

Suggestion:
"Non-nil means don't hide an entire header file enclosed by #ifndef...#endif."

> +Most C/C++ headers are usually wrapped with ifdefs to prevent re-inclusion:
> +
> +  ----- beginning of file -----
> +  #ifdef _XXX_HEADER_FILE_INCLUDED_

#ifndef?

> +  #define _XXX_HEADER_FILE_INCLUDED_
> +     xxx
> +     xxx
> +     xxx...
> +  #endif
> +  ----- end of file -----
> +
> +If we try to hide this header file, for the first time hideif will find
> +_XXX_HEADER_FILE_INCLUDED_ and have it defined.  Everything between #ifdef
> +to #endif are not hidden.

Suggestion:
"The first time we visit such a file, _XXX_HEADER_FILE_INCLUDED_ is
undefined, and so nothing is hidden.  The next time we visit it,
everything will be hidden."

> For the second time since _XXX_HEADER_FILE_INCLUDED_
> +is defined everything between the outermost #ifdef..#endif will be hidden:
> +
> +  ----- beginning of file -----
> +  #ifdef _XXX_HEADER_FILE_INCLUDED_
> +     ...
> +  #endif
> +  ----- end of file -----

I don't think the example is necessary.

> +This is not the behavior we expected, we still would like to see the content
> +of this header file.  With this flag enabled we can have the outermost #if
> +always not hidden."

Suggestion:
"This behavior is generally undesirable.  If this option is non-nil, the
outermost #if is always visible."

(I wonder: why would anyone want to set this option to nil?)

> +  :type 'boolean
> +  :version "24.4")

24.5

(Also, new options are the kind of thing to mention in a brief NEWS entry.)

> +(defcustom hide-ifdef-header-regexp
> +  "^.*\\.[hH]\\([hH]\\|[xX][xX]\\|[pP][pP]\\)?"
> +  "C/C++ header file name patterns to determine if current buffer is a 
> header.
> +Effective only if `hide-ifdef-expand-reinclusion-protection' is t."
> +  :type 'string
> +  :group 'hide-ifdef)

Missing :version.

> -     current buffer.  Initially, the global value of `hide-ifdef-env'
> -     is used.
> +        current project.  Initially, the global value of `hide-ifdef-env'
> +        is used.  This variable was a buffer-local variable which limits

"," before which

> +        hideif to parse only one C/C++ file at a time.  We've extended
> +        hideif to support parsing a C/C++ project containing multiple C/C++
> +        source files opened simultaneously in different buffers.  Therefore
> +        `hide-ifdef-env' can no longer be buffer local but must be global.
> +
> +        We can still simulate the behavior of elder hideif versions (i.e.

s/elder/older

> +        `hide-ifdef-env' being buffer local) by clearing this variable
> +        (C-c @ C) everytime before hiding current buffer.

Does all this detail need to be in the doc-string, or can it just be a
comment in the code?

(By the way, no need to make ChangeLog entries for comments.)

> +(defun hif-clear-all-ifdef-defined ()
> +  "Clears all symbols defined in `hide-ifdef-env'.
> +It will backup this variable to `hide-ifdef-env-backup' before clearing to
> +prevent accidental clearance."

Is that backup really necessary, given that you ask for confirmation?

> +  (interactive)
> +  (when (y-or-n-p "Clear all #defined symbols?")
> +    (setq hide-ifdef-env-backup hide-ifdef-env)
> +    (setq hide-ifdef-env nil)))
>  
> +  ;; Genernally there is no need to call itself recursively since there 
> should

Generally

> +           ((looking-at "\r") ; Sometimes MS-Windows user will leave CR in
> +            (forward-char 1)) ;  the source code. Let's don't stuck here.

"Let's not get stuck here."

> -(defun hif-possibly-hide ()
> +(defun hif-possibly-hide (expand-reinclusion)
>    "Called at #ifX expression, this hides those parts that should be hidden.
>  It uses the judgment of `hide-ifdef-evaluator'."

Doc should say what the argument means.


> +(defun hif-evaluate-macro (rstart rend)
> +  "Evaluate the macro expansion result for a region.
> +If no region active, find the current #ifdefs and evaluate the result. 
> Currently

Use two spaces between sentences.

> +it support only math calculations, strings or argumented macros can not be

supports

> -(defun hide-ifdef-define (var)
> +(defun hide-ifdef-define (var val)

Why not make VAL optional?

> +  "Define a VAR optionally to a specific value VAL into `hide-ifdef-env'.
> +This allows #ifdef VAR from being hidden."

Suggestion:
"Define VAR to VAL (default 1) in `hide-ifdef-env'."

> @@ -1618,11 +1860,14 @@ It does not do the work that's pointless to redo on a 
> recursive entry."
>  Assume that defined symbols have been added to `hide-ifdef-env'.
>  The text hidden is the text that would not be included by the C
>  preprocessor if it were given the file with those symbols defined.
> +If this command is prefixed, hide also the #ifdefs themselves.

Suggestion:
"With optional prefix agument ARG, also hide the #ifdefs themselves."

>    (interactive)
> -  (message "Hiding...")
> +  (let ((hide-ifdef-lines current-prefix-arg))

I think this should take an explicit prefix argument.

> +(defun hide-ifdef-block (&optional start end)
> +  "Hide the ifdef block (true or false part) enclosing or before the cursor.
> +If prefixed, it will also hide #ifdefs themselves."

Suggestion:
"With optional prefix agument ARG, also hide the #ifdefs themselves."

> +  (interactive "r")
> +  (let ((hide-ifdef-lines current-prefix-arg))

I think this should explicitly take a prefix argument.