Re: [PATCH 2/4] libcpp: Replace macro usage with C++ constructs

[Jeff Law <law at redhat dot com>]

On 05/01/2015 06:56 PM, David Malcolm wrote:
> libcpp makes extensive use of the C preprocessor.  Whilst this has a
> pleasingly self-referential quality, I find the code hard-to-read;
> implementing source location support in my JIT branch was much harder than
> I felt it should have been.
>
> In an attempt at making the code easier to follow, and to build towards
> a followup patch, this patch converts most of these macros to C++
> equivalents: using "const" for compile-time constants, and inline
> functions where macros aren't used as lvalues.
>
> This effectively documents the expected types of the params, and makes
> them available from the debugger e.g.:
>
>    (gdb) p LINEMAP_FILE ($3)
>    $1 = 0x13b8b37 "<command-line>"
>
> and indeed the constants also:
>
>    (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION)
>    $2 = false
>    (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION + 1)
>    $3 = true
>
> [I didn't mark the inline functions as "static"; should they be?]
>
> [FWIW, I posted a reduced version of this patch about a year ago as:
>    https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01092.html
> which covered a smaller subset of the macros].
>
> libcpp/ChangeLog:
> 	* include/line-map.h (MAX_SOURCE_LOCATION): Convert from a macro
> 	to a const source_location.
> 	(RESERVED_LOCATION_COUNT): Likewise.
> 	(linemap_check_ordinary): Convert from a macro to a pair of inline
> 	functions, for const/non-const arguments.
> 	(MAP_START_LOCATION): Likewise.
> 	(ORDINARY_MAP_STARTING_LINE_NUMBER): Likewise.
> 	(ORDINARY_MAP_INCLUDER_FILE_INDEX): Likewise.
> 	(ORDINARY_MAP_IN_SYSTEM_HEADER_P): Likewise.
> 	(ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): Convert from a macro to a
> 	pair of inline functions, for const/non-const arguments, where the
> 	latter is named...
> 	(SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): New function.
> 	(ORDINARY_MAP_FILE_NAME): Convert from a macro to a pair of inline
> 	functions, for const/non-const arguments.
> 	(MACRO_MAP_MACRO): Likewise.
> 	(MACRO_MAP_NUM_MACRO_TOKENS): Likewise.
> 	(MACRO_MAP_LOCATIONS): Likewise.
> 	(MACRO_MAP_EXPANSION_POINT_LOCATION): Likewise.
> 	(LINEMAPS_MAP_INFO): Likewise.
> 	(LINEMAPS_MAPS): Likewise.
> 	(LINEMAPS_ALLOCATED): Likewise.
> 	(LINEMAPS_USED): Likewise.
> 	(LINEMAPS_CACHE): Likewise.
> 	(LINEMAPS_ORDINARY_CACHE): Likewise.
> 	(LINEMAPS_MACRO_CACHE): Likewise.
> 	(LINEMAPS_MAP_AT): Convert from a macro to an inline function.
> 	(LINEMAPS_LAST_MAP): Likewise.
> 	(LINEMAPS_LAST_ALLOCATED_MAP): Likewise.
> 	(LINEMAPS_ORDINARY_MAPS): Likewise.
> 	(LINEMAPS_ORDINARY_MAP_AT): Likewise.
> 	(LINEMAPS_ORDINARY_ALLOCATED): Likewise.
> 	(LINEMAPS_ORDINARY_USED): Likewise.
> 	(LINEMAPS_LAST_ORDINARY_MAP): Likewise.
> 	(LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP): Likewise.
> 	(LINEMAPS_MACRO_MAPS): Likewise.
> 	(LINEMAPS_MACRO_MAP_AT): Likewise.
> 	(LINEMAPS_MACRO_ALLOCATED): Likewise.
> 	(LINEMAPS_MACRO_USED): Likewise.
> 	(LINEMAPS_MACRO_LOWEST_LOCATION): Likewise.
> 	(LINEMAPS_LAST_MACRO_MAP): Likewise.
> 	(LINEMAPS_LAST_ALLOCATED_MACRO_MAP): Likewise.
> 	(IS_ADHOC_LOC): Likewise.
> 	(COMBINE_LOCATION_DATA): Likewise.
> 	(SOURCE_LINE): Likewise.
> 	(SOURCE_COLUMN): Likewise.
> 	(LAST_SOURCE_LINE_LOCATION): Likewise.
> 	(LAST_SOURCE_LINE): Likewise.
> 	(LAST_SOURCE_COLUMN): Likewise.
> 	(LAST_SOURCE_LINE_LOCATION)
> 	(INCLUDED_FROM): Likewise.
> 	(MAIN_FILE_P): Likewise.
> 	(LINEMAP_FILE): Likewise.
> 	(LINEMAP_LINE): Likewise.
> 	(LINEMAP_SYSP): Likewise.
> 	(linemap_location_before_p): Likewise.
> 	* line-map.c (linemap_check_files_exited): Make local "map" const.
> 	(linemap_add): Use SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS.
> 	(linemap_line_start): Likewise.
> ---
> -#define MAP_START_LOCATION(MAP) (MAP)->start_location
> +#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007)
> +
> +/* Assertion macro to be used in line-map code.  */
> +#define linemap_assert(EXPR)                  \
> +  do {                                                \
> +    if (! (EXPR))                             \
> +      abort ();                                       \
> +  } while (0)
> +
> +/* Assert that becomes a conditional expression when checking is disabled at
> +   compilation time.  Use this for conditions that should not happen but if
> +   they happen, it is better to handle them gracefully rather than crash
> +   randomly later.
> +   Usage:
> +
> +   if (linemap_assert_fails(EXPR)) handle_error(); */
> +#define linemap_assert_fails(EXPR) __extension__ \
> +  ({linemap_assert (EXPR); false;})
> +
> +#else
> +/* Include EXPR, so that unused variable warnings do not occur.  */
> +#define linemap_assert(EXPR) ((void)(0 && (EXPR)))
> +#define linemap_assert_fails(EXPR) (! (EXPR))
> +#endif
So if we're generally trying to get away from #define programming, then 
this part seems like a bit of a step backwards.

The definition of linemap_assert in the #else clause seems odd.  Aren't 
you worried that the compiler will short-circuit the test?    If EXPR 
has side effects, then that's clearly not good.

I note you've got an abort () in here, that's generally frowned upon in 
libraries.


> -#define ORDINARY_MAP_FILE_NAME(MAP) \
> -  linemap_check_ordinary (MAP)->d.ordinary.to_file
> +/* Return TRUE if MAP encodes locations coming from a macro
> +   replacement-list at macro expansion point.  */
> +bool linemap_macro_expansion_map_p (const struct line_map *);
> +
> +/* Assert that MAP encodes locations of tokens that are not part of
> +   the replacement-list of a macro expansion.  */
> +inline struct line_map *linemap_check_ordinary (struct line_map *map)
Return type and inline declarations on a different line than the 
function name/arguments.

> -#define LINEMAPS_MAPS(SET, MAP_KIND) \
> -  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->maps
> +inline line_map *
> +LINEMAPS_MAPS (const line_maps *set, bool map_kind)
> +{
> +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
> +}
> +
> +inline line_map * &
> +LINEMAPS_MAPS (line_maps *set, bool map_kind)
> +{
> +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
> +}
Do we really need two implementation of those various functions, one for 
const line_maps * the other for line_maps *?

While I suspect ICF ultimately should merge these functions, I'd really 
prefer to avoid the duplication.

Note that many of the macros didn't have comments (they should have, but 
it's often missed), correcting that as you turn them into functions 
would be greatly appreciated.

Generally, I like what I see.  There's some minor questions/issues to 
address, so let's go with another iteration.

jeff