This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi] NFC: fix incorrect indentation of braces
ClosedPublic

Authored by zhouyizhou on Sep 7 2021, 6:22 PM.

Details

Summary

Some functions in cxa_exception_storage.cpp have incorrect indentation of braces,
use clang-format to fix them

Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>

Diff Detail

Event Timeline

zhouyizhou requested review of this revision.Sep 7 2021, 6:22 PM
zhouyizhou created this revision.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 7 2021, 6:22 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
Quuxplusone added inline comments.
libcxxabi/src/cxa_exception_storage.cpp
102

LGTM, FWIW.

It's surprising that clang-format adds } // namespace foo closing comments, but doesn't add } // extern "C" closing comments: this lonely brace is a bit confusing and I think it would benefit from an // extern "C" comment.

ldionne accepted this revision.Sep 7 2021, 6:52 PM
ldionne added a subscriber: ldionne.

FWIW, I dislike that we don't indent stuff at all inside namespaces -- often I find it useful to indent things inside short-lived namespaces. But let's go for simplicity and consistency.

Did you look into clang-formatting other files in libc++abi? What would the diff look like?

This revision is now accepted and ready to land.Sep 7 2021, 6:52 PM

FWIW, I dislike that we don't indent stuff at all inside namespaces -- often I find it useful to indent things inside short-lived namespaces. But let's go for simplicity and consistency.

Did you look into clang-formatting other files in libc++abi? What would the diff look like?

Thanks for reviewing,
I will look into other files in libc++abi one by one, and report what I have found ;-)
Thanks again
Zhouyi

libcxxabi/src/cxa_exception_storage.cpp
102

Thanks for reviewing,
clang-format do behave a little strange to me. The result of invoking clang-format in git cloned directory is diffent from invoking clang-format in other directory ( for example /tmp).
By the way, I have not write access to LLVM, so I need someone to commit for me.

Thanks again ;-)
Zhouyi

This comment was removed by zhouyizhou.

FWIW, I dislike that we don't indent stuff at all inside namespaces -- often I find it useful to indent things inside short-lived namespaces. But let's go for simplicity and consistency.

Did you look into clang-formatting other files in libc++abi? What would the diff look like?

I take a general view of .cpp(.h) files in libc++abi, they are distributed in several directories. I do an experiment with
fuzz/cxa_demangle_fuzzer.cpp using clang-format, the result is as follows:

  • cxa_demangle_fuzzer.cpp.orig 2021-09-09 02:19:17.117872938 +0800

+++ cxa_demangle_fuzzer.cpp 2021-09-09 02:19:34.845879852 +0800
@@ -1,15 +1,15 @@
-#include <stdint.h>
#include <stddef.h>
-#include <string.h>
+#include <stdint.h>
#include <stdlib.h>
-extern "C" char *
-cxa_demangle(const char *mangled_name, char *buf, size_t *n, int *status);
+#include <string.h>
+extern "C" char *
cxa_demangle(const char *mangled_name, char *buf, size_t *n,
+ int *status);

extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {

  • char *str = new char[size+1];

+ char *str = new char[size + 1];

memcpy(str, data, size);
str[size] = 0;
free(__cxa_demangle(str, 0, 0, 0));
  • delete [] str;

+ delete[] str;

return 0;

}

As a beginner of LLVM and C++, I guess clang-formatting is meaningful ;-)

I guess we can clang-format a set of files at a time, and step-by-step clang-format all the .cpp(.h) files in libc++abi and fine-tune the results as how Quuxplusone did it ,
what's your opinion? If it is OK, I am very honored and pleased to do this work ;-)

Zhouyi

Hi,
I have no write access to LLVM, can you commit D109408 for me?
Thanks a lot
Zhouyi Zhou <zhouzhouyi@gmail.com>

I was going to land this just now, but then realized that I've lost the thread of why we're doing most of this. Why are we touching clang/test/SemaCXX/? So I'm going to land just the fixing of the curly braces in libcxxabi/src/.

libcxxabi/src/cxa_exception_storage.cpp
99

This commented-out line confuses me.

This revision was landed with ongoing or failed builds.Sep 11 2021, 11:43 AM
This revision was automatically updated to reflect the committed changes.

I was going to land this just now, but then realized that I've lost the thread of why we're doing most of this. Why are we touching clang/test/SemaCXX/? So I'm going to land just the fixing of the curly braces in libcxxabi/src/.

Hi,
Very sorry to bring you so much trouble.
I am new to LLVM, and I am only intend to fix the curly braces in libcxxabi/src/,
I followed https://www.llvm.org/docs/Phabricator.html#phabricator-reviews to make a patch using:
git diff -U999999 u > /tmp/mypatch.patch
I am also very confused why clang/test/SemaCXX/ appears in mypatch.patch.
Sorry again.
And thanks for landing the fixing of the curly braces in libcxxabi/src/ for me.

Zhouyi

I was going to land this just now, but then realized that I've lost the thread of why we're doing most of this. Why are we touching clang/test/SemaCXX/? So I'm going to land just the fixing of the curly braces in libcxxabi/src/.

Hi,
Very sorry to bring you so much trouble.
I am new to LLVM, and I am only intend to fix the curly braces in libcxxabi/src/,
I followed https://www.llvm.org/docs/Phabricator.html#phabricator-reviews to make a patch using:
git diff -U999999 u > /tmp/mypatch.patch
I am also very confused why clang/test/SemaCXX/ appears in mypatch.patch.
Sorry again.
And thanks for landing the fixing of the curly braces in libcxxabi/src/ for me.

Zhouyi

When you submit patches, please follow https://libcxx.llvm.org/Contributing.html. In particular, you should follow these instructions for submitting using arc diff -- it works a lot better than the web interface: https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line.

Thanks for the patch!

I was going to land this just now, but then realized that I've lost the thread of why we're doing most of this. Why are we touching clang/test/SemaCXX/? So I'm going to land just the fixing of the curly braces in libcxxabi/src/.

Hi,
Very sorry to bring you so much trouble.
I am new to LLVM, and I am only intend to fix the curly braces in libcxxabi/src/,
I followed https://www.llvm.org/docs/Phabricator.html#phabricator-reviews to make a patch using:
git diff -U999999 u > /tmp/mypatch.patch
I am also very confused why clang/test/SemaCXX/ appears in mypatch.patch.
Sorry again.
And thanks for landing the fixing of the curly braces in libcxxabi/src/ for me.

Zhouyi

When you submit patches, please follow https://libcxx.llvm.org/Contributing.html. In particular, you should follow these instructions for submitting using arc diff -- it works a lot better than the web interface: https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line.

Thanks for the patch!

Thanks for the instructions
I will follow https://libcxx.llvm.org/Contributing.html and will follow the instructions to requesting a review via the command line.
It's a very good opportunity to improve myself ;-)
Thanks again
Zhouyi