[PATCH] D112735: export unique symbol list for xcoff with llvm-nm new option "--export-symbols"

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:667
 
+void printExportSymboList(raw_fd_ostream &OS) {
+  if (SymbolList.size() == 0)
----------------
DiggerLin wrote:
> jhenderson wrote:
> > It seems to me like you're doing several separate things in this function which are different to llvm-nm's defaults:
> > 
> > 1) Sorting based on the visibility
> > 2) Filtering out duplicates
> > 3) Filtering out symbols with certain visibilities
> > 4) Printing name and visibility columns (and not other columns like type code).
> > 5) Merging input file symbols into a single list.
> > 
> > I feel like you could achieve this in a more natural way, without needing to have a completely independent method for printing the symbols. Specifically, you could implement each of these as individual new functionality in the core logic (possibly with a user-facing option per new functionality), which "--export-symbols" then specifies automatically.
> > 
> > For example:
> > 1) The symbol visibility sorting is just another sorting mode, like --size-sort, so could be added at the point where --size-sort does something.
> > 2) Duplicate removal would be done based on all to-be-printed pieces of information, and could be done when printing there instead.
> > 3) Visibility filtering could be done at the same time as e.g. weak filtering.
> > 4) Column printing would be controlled by a series of flags (some on by default, others off by default), which control whether each column would be printed.
> > 5) Input file merging could be common behaviour.
> > 
> > The advantage of dividing things up like this is that you can implement and test each of them independently of each other, which makes reviewing much easier. Additionally, by adding some or all of these as user-level switches, it allows users to get exactly the symbols they want in the format they want.
> I think over your suggestion.  My opinion is that we should  adding new functionality or options only when it is required.
> 
> > Input file merging could be common behavior.
> 
>   I  added new function PrintMergedSymbolList() (which only print symbol name and visibility(if there is)) without adding the new option "--merge-output" .  If anyone or other object file format also need the functionality only, we can add a new patch for new option at that time  to on/off the function PrintMergedSymbolList()  and deciding which others fields need to be printed out at that time.
> 
> > Duplicate removal would be done based on all to-be-printed pieces of information, and could be done when printing there instead.
> 
>   if we add a new option  "--no-duplicate" to remove the duplication symbol, the option is no sense to a single xcoff object file(I think a single object file never has duplication symbol).  the option "--no-duplicate" only has sense on the merging output together.  I  added a new function removeDuplicateSortedSymbolList() and we can add a new option "--no-duplicate" in later patch to on/off removeDuplicateSortedSymbolList() if it is need.
> 
> > Column printing would be controlled by a series of flags 
> 
> I think this is a good idea, we can add the functionality in a separate and not urgent patch,  and current patch will not depend on it.
> 
> > The symbol visibility sorting is just another sorting mode, like --size-sort, so could be added at the point where --size-sort does something.
> 
>   I add a new functionality compareSymbolNameVisibility, it only enable on the --option "--export-symbols" . if elf object file also need the functionality,  we can add a new option to enable compareSymbolNameVisibility() at that time.
> 
> the option will  "--export-symbols" will enable the three functions at same times in current patch. 
> I think other new options only need to be added when not all of the three functions need to be enabled at the same time. 
> 
> > Visibility filtering could be done at the same time as e.g. weak filtering. 
>   Visibility only be printed at "export symbol list" and it is always need to be printed out if there is.  I will add new option "Visibility filtering"  in later patch only there is requirement.  
> 
> 
sorry , it should be
"My opinion is that we should add new functionality or options only when it is need."
in above comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112735/new/

https://reviews.llvm.org/D112735