Coding style of rustllvm

[hanna-kruppe]

Our supplementary LLVM bindings, found in the src/rustllvm/ directory, are a mess. Even basic things like brace style, naming convention, and indentation differ between functions in the same file. This makes it more painful to work with than it has to be. We should settle on a coding style and run clang-format over the file.

Which style is chosen is less important than that it's used consistently, but since that library is dealing with LLVM APIs and little else, the LLVM style seems like the obvious candidate. It's also built into clang-format so no configuration is needed.

Concrete tasks:

• Run clang-format with any style (Making code style consistent for src/rustllvm (#38688) #38701)
• Using nullptr instead of NULL (Making code style consistent for src/rustllvm (#38688) #38701)
• Consistent capitalization style for functions and types and locals (Improve naming style in rustllvm. #38745)
• Picking consistent abbreviation styles for commonly used names (e.g., Val vs Value) (Improve naming style in rustllvm. #38745)

[brson]

Agree which style is not important. If you are motivated to do it then awesome.

[brson]

Only hard part here is making a decision, but any decisions will do!

[isker]

I'll take a crack at what #38701 did not. Going with that, I'm following the LLVM style guide http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly.

[isker]

Hi @rkruppe, as a part of this I want to change the name of one FFI function in rustllvm.h, rust_llvm_string_write_impl, to LLVMRustStringWriteImpl to fit the naming scheme of the rest of the functions.

After making that change, when I go to recompile this occurs:

   Compiling proc_macro_plugin v0.0.0 (file:///Users/ikerins/rust/rust/src/libproc_macro_plugin)
error[E0523]: found two different crates with name `rustc_llvm` that are not distinguished by differing `-C metadata`. This will result in symbol conflicts between the two.
  --> src/libproc_macro_plugin/lib.rs:89:1
   |
89 | extern crate rustc_plugin;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: Could not compile `proc_macro_plugin`.

I'm sure it has something to do with my changes to librustc_llvm/lib.rs for this rename but I have no idea what the actual problem is :). This error is not documented at https://doc.rust-lang.org/error-index.html :(. Any idea? Thanks!

[hanna-kruppe]

@CannedYerins Thanks for tackling this! Unfortunately I can't think of a reason why that renaming should trigger that error. This error usually indicated something has gone awry in the build process (stale artifacts lying around), so it probably isn't due to your changes, you just hit a pre-existing bug.

It would still be nice to see if this can be reproduced reliably. Can you upload your current state so I can try building it and see if I get the same error?

Regardless, you can try a clean build (fortunately LLVM won't be rebuilt, only the Rust parts) by running x.py clean. If that makes the error disappears, everything's fine.

[isker]

@rkruppe sure. Here's my repo gzipped: https://drive.google.com/open?id=0ByYm-nqhphFaNmJ1aFpwWmFkaUE. I'm compiling x86_64-apple-darwin, if that turns out to matter...

And cleaning did fix it, by the way, thanks.

Also, it's just dawned on me that none of the FFI function names actually follow the LLVM style guide; it says camelCase where we do CamelCase. Do you think it's worth it to switch them from LLVMFooBar to llvmFooBar?

[hanna-kruppe]

Uh, right, LLVM's C++ style says camelCase. But their C API also does the LLVMCamelCase style, so we should stick to that.

Re: your repo, a commit in your fork would be easier on everyone's internet connection (not to mention faster to apply). But it's not very important, I guess it was just a fluke on your machine.

[isker]

Ah, got it.

I don't see how just a patch or a commit in my fork would help if the problem is fixed by cleaning. That's why I threw that huge thing up there.

[hanna-kruppe]

If the issue was just a mishap with the generated binaries, then other people pulling new changes and rebuilding won't be affected, and there's no problem. The only way this problem might affect others would be if getting the source code changes and building them on a pre-existing build always triggers the error. That's the case I was interested in. Everything else can fixed by cleaning once.

[isker]

Understood. I didn't realize that's what you were after. Thanks for your help on this; I ought to have a PR in a few hours.