Page MenuHomePhabricator

[Doc] Drop MSVC2013 support
ClosedPublic

Authored by mehdi_amini on Oct 17 2016, 4:16 PM.

Details

Summary

This is only updating the documentation. We'll be able to
update the codebase piece by piece.

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini retitled this revision from to [Doc] Drop MSVC2013 support.
mehdi_amini updated this object.
mehdi_amini added reviewers: jlebar, zturner, jmolloy.
mehdi_amini added a subscriber: llvm-commits.
jlebar accepted this revision.Oct 17 2016, 4:18 PM
jlebar edited edge metadata.

:happy dance:

docs/CodingStandards.rst
136 ↗(On Diff #74931)

Can we add a comment or something to the effect of "should we reconsider this now that msvc 2013 is gone?"

This revision is now accepted and ready to land.Oct 17 2016, 4:18 PM
mehdi_amini added inline comments.Oct 17 2016, 4:20 PM
docs/CodingStandards.rst
136 ↗(On Diff #74931)

Maybe we should consider it now?

jlebar added inline comments.Oct 17 2016, 4:23 PM
docs/CodingStandards.rst
136 ↗(On Diff #74931)

For my part, I have no problem with braced initializers inside a class. They can be helpful. But where possible I prefer using an equals sign, int foo_member = 42, because that's just easier to understand.

I dunno if that's contentious, though.

I was looking at these files for gcc and clang, and I think we should eliminate all redundancies. You can talk about the features, but we shouldn't mention versions except in the "supported version" section.

I've removed the mention from the developer policy, pointing to the getting started because there was more involved description there, but I really think that's the wrong place, too. Though, this can be done in a separate patch.

Cheers,
Renato

Hahnfeld added inline comments.
docs/GettingStartedVS.rst
51 ↗(On Diff #74931)

I don't think the CMake version is still valid here?

Prazek accepted this revision.Oct 18 2016, 2:01 AM
Prazek added a reviewer: Prazek.
Prazek added a subscriber: Prazek.

yey!

docs/CodingStandards.rst
136 ↗(On Diff #74931)

I also prefer = everywhere.

kparzysz added inline comments.
docs/CodingStandards.rst
136 ↗(On Diff #74931)

If MSVC was the only reason that braced initializers were disallowed then we should allow them now. Why keep that restriction after MSVC is upgraded?

Version increase in cmake/modules/CheckCompilerVersion.cmake is missing.

The reported version of Visual Studio 2015 Update 3 (the most recent) is 19.00.24215.1

mehdi_amini added inline comments.Oct 18 2016, 10:36 AM
docs/CodingStandards.rst
136 ↗(On Diff #74931)

Note: the braced initializers policy is described somewhere else: http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor

I guess it is loosely related to the issue above, but not that much. My understanding is that the rule above forbid:

class X {
  std::string Name = "<uninitialized>";
}

I don't know the motivation for it though.

This comment was removed by rengolin.

Version increase in cmake/modules/CheckCompilerVersion.cmake is missing.

The reported version of Visual Studio 2015 Update 3 (the most recent) is 19.00.24215.1

Good point.
Should we require the most recent update?

zturner edited edge metadata.Oct 18 2016, 11:10 AM

I don't know about requiring "the most recent update". But "Update 3", yes. There are some bad codegen issues in prior updates. But the wording should still say "Update 3" and not "the most recent update", because we don't want to accidentally have to bump the version in the unlikely event that an Update 4 were released, for example.

mehdi_amini edited edge metadata.

Update CheckCompilerVersion.cmake as well

Require update 3 and not the "latest" update.

(And fwiw, YAY THANK YOU ALL)

docs/CodingStandards.rst
136 ↗(On Diff #74931)

I have no idea what the motivation here was...

I am very happy with non-static class member initializers *if and only if the equals is used*.

I have fairly strong objections to using braced initialization *syntax* here. But that is just about the syntax, which this comment doesn't really mention. I would just change this to say something along the lines of:

* Feel free to use these wherever they make sense and where the `=`
  syntax is allowed. Don't use braced initialization syntax.

(and link that last bit however the links work in RST)

jmolloy edited edge metadata.Oct 19 2016, 1:00 AM

(And fwiw, YAY THANK YOU ALL)

Can we please get a rationale added for avoiding braced syntax? Not that I disagree with it, I just think things like that should be backed up in the document that bans them.

(And fwiw, YAY THANK YOU ALL)

Can we please get a rationale added for avoiding braced syntax? Not that I disagree with it, I just think things like that should be backed up in the document that bans them.

Yea, the current wording is a bit hand-wavy. I'll send out a separate patch that spiffs up the content to make this more clear.

mehdi_amini edited edge metadata.

Use Chandler wording:

Feel free to use these wherever they make sense and where the `=`
syntax is allowed. Don't use braced initialization syntax.
rnk accepted this revision.Oct 20 2016, 9:59 AM
rnk added a reviewer: rnk.
rnk added a subscriber: rnk.

looks good, but you will need to rebase, I already landed a wording update. :)

docs/GettingStartedVS.rst
48 ↗(On Diff #75161)

I kind of preferred the "latest update" wording. We don't say GCC 4.8.N, we say we support GCC 4.8, and some of those micro releases might have bugs, so make sure you're not using a buggy compiler. That's not to say that MSVC 2015 Update 1 won't work, it just isn't "supported".

Why it is not yet in the master?

Didn't take time to rebase, feel free to take over.

This revision was automatically updated to reflect the committed changes.