Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/241/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1634/ for more details.

refs http://llvm.org/viewvc/llvm-project?view=revision&revision=258128

Appears correct, as in both cases it was likely meant that FMT_USE_VARIADIC_TEMPLATES is 1 if defined(_MSC_VER) && _MSC_VER >= 1800 and same for the other one.
Agree that it would be nice to have readable logs.
Didn't understand why the replacement is different with Visual Studio than with gcc / clang though.

Didn't understand why the replacement is different with Visual Studio than with gcc / clang though.

If you're referring to why I only added another branch for VS and not done something like # if FMT_HAS_FEATURE(...)\n# define FOO 1 then that was just done to make the change minimally intrusive, and because I don't want to invest more time than needed (there are two tickets, one with code about updating to a newer version, so having the diff against the old version as minimal as possible will help with updating).

I meant the example from the commit that introduced -Wexpansion-to-undefined:

This isn't an idle threat, consider this program:
  #define FOO
  #define BAR defined(FOO)
  #if BAR
  ...
  #else
  ...
  #endif
clang and gcc will pick the #if branch while Visual Studio will take the
#else branch.

In D680#27893, @elexis wrote:

I meant the example from the commit that introduced -Wexpansion-to-undefined:

See §16.1.4 (C++11) which states

If the token defined is generated as a result of this replacement process or use
of the defined unary operator does not match one of the two specified forms prior to macro replacement,
the behavior is undefined.

So compilers (well preprocessors if we'd like to be overly technical) are free to do whatever in this case (abort, ignore it, do something that is possibly what the user wanted, do something that is what the other user wanted, ...) and a few compilers picked different users.

See N4220 which attempted to make the behaviour a little more restricted (by making expansion and handling of defined conditionally-supported), which would still have left the difference between those. Note that N4220 was not merged, and that for this specific issue there is no defect report, so this is still undefined behaviour in the most recent draft (n4659 §19.1.8).

TL;DR: Undefined behaviour, compilers doing different things

This works well and looks good, maybe we should also disable the -Wimplicit-fallthrough warning? (or add comments for that; unfortunately we don't have [[fallthrough]] until C++17)

Thanks for the review, I'll try to commit this soon.

In D680#28207, @echotangoecho wrote:

This works well and looks good, maybe we should also disable the -Wimplicit-fallthrough warning? (or add comments for that; unfortunately we don't have [[fallthrough]] until C++17)

I'd rather not disable that warning, the few cases we have could either be fixed by slightly adjusting the comments (only one occurence misses one IIRC), or by adding a macro.

However I've been pondering whether we shouldn't just use [[fallthrough]]; where supported and define that as something that is supported by older compilers, or just as nothing in case there is no equivalent.

Clang has [[clang::fallthrough]] since 3.2 or something (but at least 3.4 (which is our lowest supported version has [[clang::fallthrough]] [1]), and 3.9 introduced [[fallthrough]] [2] for C++11 onwards.
GCC 7 [3] supports [[gnu::fallthrough]] for C++11 and C++14 (and [[fallthrough]] for C++17 and later.
MSVC++ supports [[fallthrough]] since VS2017, but needs a special flag to enable it [5]. However one could use __fallthrough which allegedly exists and is used by their code analyzer [6].
Since those are all really supported compilers (ignore hints at ICC in the code since that is most likely broken by now, unless someone shows me a build log...) we could just check if [[fallthrough]] is supported (__has_cpp_attribute [7]) and if not reaplace it by something that is

#ifdef __has_cpp_attribute
# if __has_cpp_attribute(fallthrough)
// nothing
# elif __has_cpp_attribute(gnu::fallthrough)
#  define [[fallthrough]] [[gnu::fallthrough]]
# elif __has_cpp_attribute(clang::fallthrough)
#  define [[fallthrough]] [[clang::fallthrough]]
# else
#  define [[fallthrough]]
# endif
#else
# define [[fallthrough]]
#endif
//TODO: Maybe use __fallthrough for the MSVC code analyzer (also figure out if we need to add some switch when switching to a newer version of VS that supports [[fallthrough]]

[1] http://releases.llvm.org/3.4/tools/clang/docs/LanguageExtensions.html
[2] http://releases.llvm.org/3.9.0/tools/clang/docs/ReleaseNotes.html#c-1z-feature-support
[3] At least I haven't found any indication that any of those was supported before that (but I've dug through Clang release notes and documentation for about an hour, so I'll just assume GCC 7 since earlier versions don't warn, and a blog post [4] by the author of the patch that added the warning seems to indicate that it is indeed GCC 7.
[4] https://dzone.com/articles/implicit-fallthrough-in-gcc-7
[5] https://docs.microsoft.com/en-us/cpp/visual-cpp-language-conformance
[6] If someone can find proof of its existence and if we need to include something for that, please post it. I found usage of it in Z3 that implies that it is a macro, and some discussion about using it for MOZ_FALLTHROUGH on some mozilla news group.
[7] Supported since GCC 5. I'm relatively certain that clang supports that for quite a while. I don't think MSVC++ has that.