[PATCH] D89987: [analyzer] [NFC] Rename SymbolRef to SymExprRef
Balázs Benics via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 23 09:21:33 PDT 2020
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
More information about the cfe-commits
mailing list