Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13233 closed defect (implemented)

Drop c90 compiler support

Reported by: Sebastian Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: nickm-patch
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I think it would be reasonable to add a configure check which complains if all your compiler supports is c90, and add a switch to override that behaviour documenting that if you use it without telling us, Tor will stop working for you with 0.2.7. That way, we don't prevent anyone from building Tor today, we still enfore c90 code for now in 0.2.6 without breaking it accidentally like we certainly would if we just lifted the requirement, and have a smooth way forward. Happy to cook up a patch if people think this is a good idea.

Child Tickets

Change History (15)

comment:1 Changed 5 years ago by nickm

Sounds okay to me. I wish we could drop the c90 requirement right now, and let anybody who doesn't like it complain... but the risk there is that if we guess wrong and find out that there is some C90 compiler that we need to support, we would maybe need to adjust a lot of code. Maybe it wouldn't be too bad, though?

Also, to be clear: The only c99 feature we're planning to require is the ability to declare variables in the middle of a block, right?

comment:2 in reply to:  1 Changed 5 years ago by Sebastian

Ah, I thought it would be more of a slow migration towards C99, definitely starting with the variable declaration in the middle of blocks, but also more eventually. If it is just that I think we can just switch - if we make a mistake refactorings will be not so hard.

comment:3 Changed 5 years ago by nickm

Are there other c99 features you had in mind?

comment:4 Changed 5 years ago by Sebastian

no, not immediately. But my thoughts were that if someone had a patch we'd not reject it because it used c99.

comment:5 Changed 5 years ago by nickm

Here's an imperfect coccinelle script to transform our code to use c99-style declare-at-time-of-use. Probably we shouldn't run it for a while, until we're certain we will allow this syntax long-term.

@@
identifier func;
identifier v;
type tp;
expression e, e2;
statement S1;
@@
 func(...) {
 ...
-tp v;
 ... when != v
(
 {
   ... v ...
 }
|
 if (e2) { ... v ... } else S1;
|
 if (e2) S1 else { ... v ... }
|
 if (e2) { ... v ... }
|
+tp
 v = e;
 ...
)
 }

comment:6 Changed 5 years ago by Sebastian

Do you think we should switch like that? I thought we'd just naturally transition over. Also, what about the previous comment?

comment:7 in reply to:  6 Changed 5 years ago by nickm

Replying to Sebastian:

Do you think we should switch like that? I thought we'd just naturally transition over.

It's an option! Naturally transitioning for a while is probably a good idea, though.

Also, what about the previous comment?

You mean the one about other c99 features? I think we should be careful there; the only one we've really wanted so far has been the mid-block declarations. If we find out that all compilers have those, it might not mean that all compilers have designated initializers. Or we might decide that (say) compound literals are silly. So let's go on a case-by-case basis.

comment:8 Changed 5 years ago by nickm

Keywords: nickm-patch added
Status: newneeds_review

I've added a sample branch in my public repository as "require_some_c99". How does it look?

comment:9 Changed 5 years ago by Sebastian

Looks good to me. I didn't find a compiler on any of my systems which didn't like it, but I don't keep too many obscure things around.

comment:10 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged! Opened another ticket (#13260) for code changes later in this series (or later still).

comment:11 Changed 5 years ago by NewEraCracker

Hello,
Unfortunately C99 is not supported by Visual Studio 2010.

Is it possible for you to revert this?

For example, the following code from address.c in common dir will not compile:

tor_assert(addr);
sa_family_t v_family = tor_addr_family(addr);

v_family needs to be declared before function call.

struct in_addr in_tmp = { .s_addr = 0 };

a c89 way to do this is to use { 0 } instead, it is equivalent to a memset but shouldn't cause much problem here.

static char nil_bytes[16] = { [0]=0,0,0,0, 0,0,0,0, 0,0,0,0, 0,0,0,0 };

there are 16 elements in that array, and 16 zeros declared, why using [0]= ?

Regards,
NewEraCracker

comment:12 Changed 5 years ago by nickm

Have a look at commit message 0ca83872468af59 -- those C99 features were put into address.c specifically to catch compilers without c99 support, so that we can find out from people whether there's a compelling need not to use the 15-year-old standard.

There's been a thread about this on tor-dev last month. In particular, see https://lists.torproject.org/pipermail/tor-dev/2014-October/007633.html .

comment:13 Changed 5 years ago by nickm

Hm. Actually, do all the other modules build with VS 2010 ? If so, maybe we could restrict ourselves to a disappointing but not wholly inadequate subset subset of c99...

comment:14 Changed 5 years ago by NewEraCracker

Yes! I have even created a newer patches to build Tor with MSVC 2010.

See #13081 for more details.

comment:15 Changed 5 years ago by nickm

That second patch actually seems to suggest that the other modules don't build with MSVC 2010. It looks like some of the change you're doing are putting declarations be at the start of their own blocks, according to c90 style. That issue is the big one that made me want to shift away from c90 in the first place.

Note: See TracTickets for help on using tickets.