[PATCH] D74023: [RISCV] ELF attribute section for RISC-V

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 24 09:26:18 PST 2020


MaskRay added subscribers: SjoerdMeijer, samparker, compnerd.
MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Support/ELFAttributeParser.h:31
+
+  uint64_t ParseInteger(const uint8_t *Data, uint32_t &Offset);
+  StringRef ParseString(const uint8_t *Data, uint32_t &Offset);
----------------
jhenderson wrote:
> MaskRay wrote:
> > Use `camelCase` function names.
> > 
> > You can even start to use `variableName` for variables. Because this patch adds many new files. They don't need to get stuck with existing `VariableName` rule which is considered broken  https://lists.llvm.org/pipermail/llvm-dev/2019-February/129907.html
> @MaskRay, re. variable names, whilst the existing scheme is indeed considered broken, I wasn't aware of any agreement that new code should follow the proposed new style?
@jhenderson @samparker @compnerd @SjoerdMeijer 

For existing code, there is objection that clean-ups can clutter up the history. There is a supportive group of people putting forward a migration plan. For `*AttributeParser`, I think it is a middle ground: the rewrite (refactoring) will touch nearly every line (we will also face a large function move ARMAttributeParser.cpp -> ELFAttributeParser.cpp). I am in the camp of "I don't want to unnecessarily break the style of surrounding code, but if it is new code, I want to just use the ideal style." So I went forward and fixed the variable Naming in D75015.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023





More information about the llvm-commits mailing list