[PATCH] D68049: Propeller: Clang options for basic block sections

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:120
 
+  std::string BasicBlockSections;
+
----------------
MaskRay wrote:
> Comment its allowed values ("all", "labels", "none")
I'd suggest to rewrite it somehow. This set of values did not help me to understand what is this field for. The comment could probably be (for example): "This is a field for.... Allowed values are:...."


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:444
+  while ((std::getline(fin, line)).good()) {
+    StringRef S(line);
+    // Lines beginning with @, # are not useful here.
----------------
Something is wrong with the namings (I am not an expert in lib/CodeGen), but you are mixing lower vs upper case styles: "fin", "line", "S", "R". Seems the code around prefers upper case.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:450
+      break;
+    if (S.consume_front("!")) {
+      if (fi != Options.BBSectionsList.end())
----------------
```
    if (S.empty() || S[0] == '@' || S[0] == '#')
      continue;
    if (!S.consume_front("!") || S.empty())
      break;
    if (S.consume_front("!")) {
```

It is looks a bit strange. See: you are testing `S.empty()` condition twice and you are checking `S.consume_front("!")` twice.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:461
+      fi = R.first;
+      assert(R.second);
+    }
----------------
It seems this assert can be triggered for a wrong user input?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:964
+        << Opts.BBSections;
+  }
+
----------------
Doesn't seem you need "{}" around this lines. (Its a single call and looks inconsistent with the code around).
(The same for the change above)


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

https://reviews.llvm.org/D68049