[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D123345#3452496 <https://reviews.llvm.org/D123345#3452496>, @aaron.ballman wrote:

> Do you have ideas on how we can improve the debugging checkpoint behavior (if at all)?

I think we just live with it, like we do for other builtin functions. (There might be things we can do by emitting inlining info into the debug info. If we do that, we should presumably do it for all builtin lib functions.)



================
Comment at: clang/test/SemaCXX/builtin-std-move.cpp:12
+
+  template<typename T> CONSTEXPR T &&move(T &x) {
+    static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
----------------
aaron.ballman wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > Formatting looks like it's gone wonky in this file.
> > What are you referring to? The lines longer than 80 columns, or something else?
> > 
> > FWIW, long lines are common in our `test/` files. Substantially more than half of them go over 80 columns:
> > ```
> > $ rgrep '.\{81\}' clang/test  --files-with-match | wc -l
> > 12481
> > $ rgrep '.\{81\}' clang/test  --files-without-match | wc -l
> > 7194
> > ```
> Oh, the line length doesn't bother me (we smash all over that in tests, as you've seen). It was more the extra indentation within a namespace -- we don't usually indent like that and it caught me off guard. It's not a major deal though (the same indentation happens in clang/test/SemaCXX/builtin-std-move-nobuiltin.cpp).
Gotcha.

This is just what my editor / fingers happened to do, but now I'm consciously thinking about it, I see a bit of nuance here: I think it makes sense to not indent a namespace that spans a whole file, because the indent doesn't add anything for the reader and loses two columns of useful screen real estate. But when the namespace is covering a relatively small scope it seems useful to me to indent it to contrast the contents of the namespace against the surrounding code that's outside the namespace, in the same way it makes sense to indent a class definition to contrast the class scope against surrounding code.

It looks like indenting namespace bodies is pretty common in the clang test suite: of the 1917 files that contain a line matching /^namespace.*$/, 1171 of those files have a next line that is indented, which I think makes sense because namespaces in tests will tend to be short, and intended to semantically separate a portion of the test from the rest of the file. (By contrast, only ~10% of the clang sources and headers have indented namespaces, and from a quick sampling it looks like they basically follow the same pattern: indented namespaces are mostly used for short scopes and unindented ones are mostly used for longer / whole-file scopes.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345