diff mbox series

[1/2] Add --header-file option to add individual public header files.

Message ID 87h7xmnlhh.fsf@seketeli.org
State Committed
Headers show
Series [1/2] Add --header-file option to add individual public header files. | expand

Commit Message

Dodji Seketeli April 14, 2020, 3:19 p.m. UTC
Hello Mark,

Mark Wielaard <mark@klomp.org> a ?crit:

> Sometimes public header files are in the same directory as private
> header files. In such cases --headers-dir cannot be used. Add
> --header-file to add individual public header files.
>
> 	* include/abg-tools-utils.h (gen_suppr_spec_from_headers): Add
> 	hdr_files string vector argument.
> 	* src/abg-tools-utils.cc (handle_file_entry): New function that
> 	adds one specific file to the type_suppression. Implementation
> 	lifted from...
> 	(handle_fts_entry): ...here. Call handle_file_entry for each file.
> 	(gen_suppr_spec_from_headers): Also takes a header_files string
> 	vector as argument. Call handle_file_entry for each file entry.
> 	* tools/abidiff.cc (options): Add header_files1 and header_files2
> 	string vectors.
> 	(display_usage): Print --header-file1 and --header-file2 usage.
> 	(parse_command_line): Handle --header-file1, --hf1 and
> 	--header-file2, --hf2.
> 	(set_diff_context_from_opts): Call gen_suppr_spec_from_headers
> 	with header_files1 and header_files2.
> 	(set_suppressions): Likewise.
> 	* tools/abidw.cc (options): Add header_files string vector.
> 	(display_usage): Print --header-file usage.
> 	(parse_command_line): Handle --header-file1, --hf1.
> 	(maybe_check_header_files): New function.
> 	(set_suppressions): Call gen_suppr_spec_from_headers with
> 	header_files.
> 	(main): Call maybe_check_header_files.
> 	* tools/abilint.cc (options): Add header_files string vector.
> 	(display_usage): Print --header-file usage.
> 	(parse_command_line): Handle --header-file1, --hf1.
> 	(set_suppressions): Call gen_suppr_spec_from_headers with
> 	header_files.
> 	* tools/abipkgdiff.cc (maybe_create_private_types_suppressions):
> 	Call gen_suppr_spec_from_headers with an empty string vector.
> 	* doc/manuals/abidiff.rst: Document --header-file1, --hf1 and
> 	--header-file2, --hf2. Add new options to documentation of
> 	--drop-private-types.
> 	* doc/manuals/abidw.rst: Document --header-file, --hf.
> 	* doc/manuals/abilint.rst: Likewise.

Thanks for this nice patch!  I have applied it to master!

I have just made some cosmetic changes to a few parts that I am going to
point to below.  Also, after my comments of your patch, I have attached
a diff of the changes I made to it.  I have also attached the resulting
amended patch.

[...]

> diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
> index 611b4d07..ea4efb0e 100644
> --- a/tools/abipkgdiff.cc
> +++ b/tools/abipkgdiff.cc
> @@ -1529,8 +1529,10 @@ maybe_create_private_types_suppressions(package& pkg, const options &opts)
>    if (!is_dir(headers_path))
>      return false;
>  
> +  // We don't list individual files, just look under the headers_path.
> +  vector<string> no_header_files;
>    suppression_sptr suppr =
> -    gen_suppr_spec_from_headers(headers_path);
> +    gen_suppr_spec_from_headers(headers_path, no_header_files);

In order to avoid this change, I have added an overload for the
gen_suppr_spec_from_headers function that takes just one parameter, like
it was before your patch.  That way, this hunk becomes unnecessary.  You
can see the details for that in the diff representing my changes at the
end of this message.

[...]

> diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
> index 11f6129e..b05ed645 100644
> --- a/src/abg-tools-utils.cc
> +++ b/src/abg-tools-utils.cc
> @@ -1787,6 +1787,40 @@ make_path_absolute_to_be_freed(const char*p)
>    return result;
>  }
>  
> +/// This is a sub-routine of gen_suppr_spec_from_headers and
> +///  handle_fts_entry.
> +///
> +/// @param file add to the to the vector returned by
> +/// type_suppression::get_source_locations_to_keep()
> +///
> +/// @param suppr the file name is going to be added to the vector
> +/// returned by the method type_suppression::get_source_locations_to_keep
> +/// of this instance.
> +/// If this smart pointer is nil then a new instance @ref
> +/// type_suppression is created and this variable is made to point to
> +/// it.

I have slightly edited the comments above.
> +static void
> +handle_file_entry(const string& file,
> +		  type_suppression_sptr& suppr)
> +{

[...]

> @@ -1845,25 +1863,35 @@ handle_fts_entry(const FTSENT *entry,
>  /// @return the resulting type suppression generated, if any file was
>  /// found in the directory tree @p headers_root_dir.
>  type_suppression_sptr
> -gen_suppr_spec_from_headers(const string& headers_root_dir)
> +gen_suppr_spec_from_headers(const string& headers_root_dir,
> +			    const vector<string>& header_files)

I have added a comment for the new parameter of this function.

>  {

[...]

>    type_suppression_sptr result;
>  
> -  if (headers_root_dir.empty())
> -    // We were given no headers root dir so the resulting suppression
> -    // specification shall be empty.
> +  if (headers_root_dir.empty() && header_files.empty())
> +    // We were given no headers root dir and no header files
> +    // so the resulting suppression specification shall be empty.
>      return result;
>  
> -  char* paths[] = {const_cast<char*>(headers_root_dir.c_str()), 0};
> +  if (!headers_root_dir.empty())
> +    {
> +      char* paths[] = {const_cast<char*>(headers_root_dir.c_str()), 0};
>  
> -  FTS *file_hierarchy = fts_open(paths, FTS_LOGICAL|FTS_NOCHDIR, NULL);
> -  if (!file_hierarchy)
> -    return result;
> +      FTS *file_hierarchy = fts_open(paths, FTS_LOGICAL|FTS_NOCHDIR, NULL);
> +      if (!file_hierarchy)
> +	return result;
> +
> +      FTSENT *entry;
> +      while ((entry = fts_read(file_hierarchy)))
> +	handle_fts_entry(entry, result);
> +      fts_close(file_hierarchy);
> +    }
> +
> +  for (vector<string>::const_iterator file = header_files.begin();
> +       file != header_files.end();
> +       ++file)
> +    handle_file_entry(*file, result);
>  
> -  FTSENT *entry;
> -  while ((entry = fts_read(file_hierarchy)))
> -    handle_fts_entry(entry, result);
> -  fts_close(file_hierarchy);
>    return result;
>  }
>

[...]


Here is the diff representing my changes to your patch.

--------------------------------->8<-------------------------------

--------------------------------->8<-------------------------------

And below is the final resulting patch that I have applied to master.

Thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-header-file-option-to-add-individual-public-head.patch
Type: text/x-patch
Size: 22614 bytes
Desc: Final amended patch
URL: <https://sourceware.org/pipermail/libabigail/attachments/20200414/7be7e898/attachment-0001.bin>
-------------- next part --------------
diff mbox series

Patch

diff --git a/include/abg-tools-utils.h b/include/abg-tools-utils.h
index 564fe646..c47e3f4e 100644
--- a/include/abg-tools-utils.h
+++ b/include/abg-tools-utils.h
@@ -83,6 +83,10 @@  string trim_white_space(const string&);
 string trim_leading_string(const string& from, const string& to_trim);
 void convert_char_stars_to_char_star_stars(const vector<char*>&,
 					   vector<char**>&);
+
+suppr::type_suppression_sptr
+gen_suppr_spec_from_headers(const string& hdrs_root_dir);
+
 suppr::type_suppression_sptr
 gen_suppr_spec_from_headers(const string& hdrs_root_dir,
 			    const vector<string>& hdr_files);
diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index b05ed645..a06e8615 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -1788,19 +1788,22 @@  make_path_absolute_to_be_freed(const char*p)
 }
 
 /// This is a sub-routine of gen_suppr_spec_from_headers and
-///  handle_fts_entry.
+/// handle_fts_entry.
 ///
-/// @param file add to the to the vector returned by
+/// It setups a type suppression which is meant to keep types defined
+/// in a given file and suppress all other types.
+///
+/// @param file_path the path to the file that defines types that are
+/// meant to be kept by the type suppression.  All other types defined
+/// in other files are to be suppressed.  Note that this file path is
+/// added to the vector returned by
 /// type_suppression::get_source_locations_to_keep()
 ///
-/// @param suppr the file name is going to be added to the vector
-/// returned by the method type_suppression::get_source_locations_to_keep
-/// of this instance.
-/// If this smart pointer is nil then a new instance @ref
-/// type_suppression is created and this variable is made to point to
-/// it.
+/// @param suppr the type suppression to setup.  If this smart pointer
+/// is nil then a new instance @ref type_suppression is created and
+/// this variable is made to point to it.
 static void
-handle_file_entry(const string& file,
+handle_file_entry(const string& file_path,
 		  type_suppression_sptr& suppr)
 {
   if (!suppr)
@@ -1818,7 +1821,7 @@  handle_file_entry(const string& file,
   // And types that are defined in header files that are under
   // the header directory file we are looking are to be
   // considered public types too.
-  suppr->get_source_locations_to_keep().insert(file);
+  suppr->get_source_locations_to_keep().insert(file_path);
 }
 
 /// This is a sub-routine of gen_suppr_spec_from_headers.
@@ -1854,12 +1857,16 @@  handle_fts_entry(const FTSENT *entry,
 }
 
 /// Generate a type suppression specification that suppresses ABI
-/// changes for types defines in source files that are *NOT* in a give
-/// header root dir.
+/// changes for types defined in source files that are neither in a
+/// given header root dir, not in a set of header files.
 ///
 /// @param headers_root_dir ABI changes in types defined in files
 /// *NOT* found in this directory tree are going be suppressed.
 ///
+/// @param header_files a set of additional header files that define
+/// types that are to be kept (not supressed) by the returned type
+/// suppression.
+///
 /// @return the resulting type suppression generated, if any file was
 /// found in the directory tree @p headers_root_dir.
 type_suppression_sptr
@@ -1895,6 +1902,23 @@  gen_suppr_spec_from_headers(const string& headers_root_dir,
   return result;
 }
 
+/// Generate a type suppression specification that suppresses ABI
+/// changes for types defined in source files that are not in a given
+/// header root dir.
+///
+/// @param headers_root_dir ABI changes in types defined in files
+/// *NOT* found in this directory tree are going be suppressed.
+///
+/// @return the resulting type suppression generated, if any file was
+/// found in the directory tree @p headers_root_dir.
+type_suppression_sptr
+gen_suppr_spec_from_headers(const string& headers_root_dir)
+{
+  // We don't list individual files, just look under the headers_path.
+  vector<string> header_files;
+  return gen_suppr_spec_from_headers(headers_root_dir, header_files);
+}
+
 /// Generate a suppression specification from kernel abi whitelist
 /// files.
 ///
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index ea4efb0e..611b4d07 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -1529,10 +1529,8 @@  maybe_create_private_types_suppressions(package& pkg, const options &opts)
   if (!is_dir(headers_path))
     return false;
 
-  // We don't list individual files, just look under the headers_path.
-  vector<string> no_header_files;
   suppression_sptr suppr =
-    gen_suppr_spec_from_headers(headers_path, no_header_files);
+    gen_suppr_spec_from_headers(headers_path);
 
   if (suppr)
     {