curdeius added a comment.

@MyDeveloperDay , I know it's a strange request, but could you change (or 
remove) the background color for 100% case.
I'm partially color-blind and having red and green background in the same table 
is really hard to distinguish. I guess I'm not alone.
I'd suggest using something like light blue, it doesn't need to stand out 
anyway.



================
Comment at: clang/docs/tools/generate_formatted_state.py:50
+    totalFilesFail = 0
+    for root, subdirs, files in os.walk(rootdir):
+        path = os.path.relpath(root, LLVM_DIR)
----------------
Unused `subdirs` variable: change to `_`.


================
Comment at: clang/docs/tools/generate_formatted_state.py:52
+        path = os.path.relpath(root, LLVM_DIR)
+        if "/test/" in path:
+            continue
----------------
That doesn't work on Windows because of slashes. You doesn't skip `unittests` 
(present at least in clang and llvm).


================
Comment at: clang/docs/tools/generate_formatted_state.py:56
+        while head:
+            fileCount = 0
+            filePass = 0
----------------
curdeius wrote:
> You can move it outside the loop.
Here you use camelCase, but in other places you use snake_case (e.g. 
`file_path`). Please make that consistent.


================
Comment at: clang/docs/tools/generate_formatted_state.py:56-58
+            fileCount = 0
+            filePass = 0
+            fileFail = 0
----------------
You can move it outside the loop.


================
Comment at: clang/docs/tools/generate_formatted_state.py:80
+        if (fileCount > 0):
+            output.write("   * - " + path + "\n")
+            output.write("     - " + str(fileCount) + "\n")
----------------
Writing `path` directly on Windows will put backslashes that are not rendered 
in rst, so you should either change backslashes to forward slashes (that's what 
I'd suggest) or double the backslashes. You can just `path.replace('\\', '/')`.


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

https://reviews.llvm.org/D80627



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to