[PATCH] D57896: Variable names rule

Csaba Dabis via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 19 07:49:17 PDT 2019


Charusso added subscribers: chandlerc, Charusso.
Charusso added a comment.

In D57896#1406812 <https://reviews.llvm.org/D57896#1406812>, @lattner wrote:

> I can understand Zach's position here, but LLDB has historically never conformed to the general LLVM naming or other conventions due to its heritage.  It should not be taken as precedent that the rest of the project should follow.
>
> In any case, I think that it is best for this discussion to take place on the llvm-dev list where it is likely to get the most visibility.  Would you mind moving comments and discussions there?


Hey! Random Clang developer is here after this topic became a little-bit dead as not everyone subbed to LLVM dev-list. I think the best solution for every difficult question is to let the users decide their own future among all the projects. I would announce polls (https://reviews.llvm.org/vote/) and announce them on every dev-list.

I do not see any better solution to decide if we would code like `DRE`, `VD` versus `expr`, `decl` as @lattner would code. And I am not sure if everyone happy with `this_new_styling` as @chandlerc and @zturner would code. E.g. I am not happy because I know my if-statements would take two lines of code instead of one and it would be super-duper-mega ugly and difficult to read. Here is an example:

  static Optional<const llvm::APSInt *>
  getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
  //...
    if (const auto *DRE = dyn_cast_or_null<DeclRefExpr>(CondVarExpr)) {
      if (const auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {
  //...
  }

would be:

  static Optional<const llvm::APSInt *>                                           |
  getConcreteIntegerValue(const Expr *cond_var_expr, const ExplodedNode *node) {  |
  //...                                                                           |
    if (const auto *decl_ref_expr = dyn_cast_or_null<DeclRefExpr>(cond_var_expr)) {
      if (const auto *var_decl = dyn_cast_or_null<VarDecl>(decl_ref_expr->getDecl())) {
  //...                                                                           |
  }                                                             whoops column-81 ~^

Hungarian notation on members and globals are cool idea. However, the notation is made without the `_` part, so I think `mMember` is better than `m_member` as we used to 80-column standard and it is waste of space and hurts your C-developer eyes. I would recommend `b` prefix to booleans as Unreal Engine 4 styling is used to do that (`bIsCoolStyle`) and it is handy. It is useful because booleans usually has multiple prefixes: `has, have, is` and you would list all the booleans together in autocompletion. Yes, there is a problem: if the notation is not capital like the pure Hungarian notation then it is problematic to list and we are back to the `BIsCapitalLetter` and `MMember` CamelCase-world where we started (except one project). I think @lattner could say if it is useful as all the Apple projects based on those notations and could be annoying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896





More information about the cfe-commits mailing list