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

[Glenn Morris]

Some stylistic comments only:

It needs a ChangeLog entry, and probably a NEWS entry.

> -;;   Daniel LaLiberte <liberte@holonexus.org>
> +;;      Daniel LaLiberte <liberte@holonexus.org>

Please don't change existing whitespace in areas that you are not
otherwise touching.

> -;;    (unless hide-ifdef-define-alist
> -;;      (setq hide-ifdef-define-alist
> -;;           '((list1 ONE TWO)
> -;;             (list2 TWO THREE))))
> -;;    (hide-ifdef-use-define-alist 'list2))) ; use list2 by default
> +;;       (unless hide-ifdef-define-alist
> +;;         (setq hide-ifdef-define-alist
> +;;              '((list1 ONE TWO)
> +;;                (list2 TWO THREE))))
> +;;       (hide-ifdef-use-define-alist 'list2))) ; use list2 by default

Again, this is just whitespace.

> @@ -129,16 +129,44 @@
>    "Non-nil means shadow text instead of hiding it."
>    :type 'boolean
>    :group 'hide-ifdef
> -  :version "23.1")
> +  :version "24.5")
>  
>  (defface hide-ifdef-shadow '((t (:inherit shadow)))
>    "Face for shadowing ifdef blocks."
>    :group 'hide-ifdef
> -  :version "23.1")
> +  :version "24.5")

Why is the :version changing, when the defaults are unchanged?

>   (defcustom hide-ifdef-exclude-define-regexp nil
>    "Ignore #define names if those names match this exclusion pattern."
>    :type 'string)
> +(defcustom hide-ifdef-expand-reinclusion-protection t
> +  "When hiding header files, enabling this flag allows hideif always try to
> +expand the re-inclusion protected ifdefs.  Disabling this flag those headers
> +are usually hidden to a top level #ifdef...#endif due to those defined 
> symbols

The first line of a doc-string should be a complete sentence that fits
in < 80 columns. All the doc should fit within the standard fill-column.

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

Again, the first line of the doc should be a complete sentence.
New defcustoms need :version tags (and probably NEWS entries).
 
> +(defvar hide-ifdef-env-backup nil
> +  "A backup variable to prevent `hide-ifdef-env' accidentally cleared by
> +`hif-clear-all-ifdef-defined'.")

First line of doc too long. Also, this is ungrammatical.

>  `hide-ifdef-env'
> -     An association list of defined and undefined symbols for the
> -     current buffer.  Initially, the global value of `hide-ifdef-env'
> -     is used.
> +        An association list of defined and undefined symbols for the
> +        current project.  Initially, the global value of `hide-ifdef-env'
> +        is used.  This variable was a buffer-local variable but is now a
> +        global variable since we've extend hideif to support project-based

s/extend/extended.
"project-based across all-buffers" doesn't make sense.
I'm not sure that describing how things used to work is helpful.

> +        across all-buffers.  To simulate the original buffer local behavior
> +        we need to clear this variable (C-c @ C) then hide current buffer.

>  `hide-ifdef-define-alist'
> -     An association list of defined symbol lists.
> +        An association list of defined symbol lists.

whitespace.

> -     Set to non-nil to not show #if, #ifdef, #ifndef, #else, and
> -     #endif lines when hiding.
> +        Set to non-nil to not show #if, #ifdef, #ifndef, #else, and
> +        #endif lines when hiding.

whitespace.


At this point, I'll give up, and ask you to send a version that does not
have pointless whitespace changes. :)