steakhal added a comment. In D89987#2349959 <https://reviews.llvm.org/D89987#2349959>, @ASDenysPetrov wrote:
> @OikawaKirie > >> Different from ProgramStateRef which is an alias to IntrusiveRefCntPtr, or >> StoreRef which is a wrapper object, an alias to a const SymExpr * makes no >> sense to me. > > Yes. I omit this, because in such case we should go further and rename all > those which are not real `Ref` to `Ptr` or smth that would emphasise that > it's just a pointer alias, not a wrapper or another class. > That's why I prefered to change the name a little in favor of complex > approach of renaming all the rest. > >> And this is also where I have been confused for a long while. > > So have been I. The patch is called to make it more clear :) > > Thanks to @steakhal comment I investigated more in terms of what other names > use `Symbol` but mean `SymExpr`. They are: > > class SymbolManager; > using SymbolID; > using SymbolDependTy; > class SymbolData; > class SymbolMetadata; > class SymbolReaper; > enum SymbolStatus; > using SymbolSetTy; > using SymbolMapTy; > class SymbolCast; > class SymbolVal; > class symbol_iterator; > etc. > > This is not a full list! I also didn't count //methods// and //file names//. > There is also a list of objects names which straightly use `SymExpr`. They > are less spread. Mostly they are derived classes: > > class BinarySymExprImpl; > class SymIntExpr; > class IntSymExpr; > class SymSymExpr; > some enums, several methods, etc. > > As a result we should accurately define the difference between `Symbol` and > `SymExpr`. There are some cases, when `Symbol` refers to the base-class of the inheritance hierachy. In those cases the renaming would be appropiate. Though, I'm sure there are cases, when the `Symbol` refers simply to `SymbolData` - in which cases we should rename accordingly. At the previous LLVM SA round table @xazax.hun also mentioned that we could have chosen better names. Maybe it's the time to have consensus about this. > I see the next options: > > 1. `Symbol` and `SymExpr` are different. Leave the names as are. Fix minor > mismatches if presented. And follow that definitions. > 2. `Symbol` and `SymExpr` are the same. `SymExpr` is a dominant one. Change > all the names from `Symbol` to `SymExpr` and get rid of `Symbol`. > 3. `Symbol` and `SymExpr` are the same. `Symbol` is a dominant one. Change > all the names from `SymExpr` to `Symbol` and get rid of `SymExpr`. > 4. Ignore the naming due to loss of **git blame** (or any other reason) like > in an llvm naming rules topic. Losing **git blame** would have a signifficant impact in deed, however we always have the option to add the renaming commit to the `.git-blame-ignore-revs`. To achieve that, we should probably have a better reason behind it besides simply //renaming// stuff. > 5. For now just rename `SymbolRef` alias as the beggining of the story in > scope of 2nd option. > > What do you think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89987/new/ https://reviews.llvm.org/D89987 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
