[00:11:48] --- dev-zero@jabber.org has left [00:16:03] --- haba has left [00:44:12] --- dev-zero@jabber.org has become available [00:52:40] --- sxw has become available [03:27:50] --- sxw has left [04:54:27] --- Jeffrey Altman has left [04:54:31] --- Jeffrey Altman has become available [05:33:57] --- Jeffrey Altman has left: Replaced by new connection [05:35:09] --- Jeffrey Altman has become available [05:53:18] --- sxw has become available [06:12:57] --- cclausen has become available [07:43:55] When someone votes +1 or -1. or a gatekeeper votes +2 or -2. Who are the messages directed to? I think the misunderstanding is a result of a misunderstanding of the target of the message. [07:44:20] Are the messages targeted at the submitter? Or are they targeted at the gatekeepers? [07:45:05] A message "-1 Please do not merge this in its current form" clearly has the intent that it is aimed at the gatekeepers. [07:47:11] Since +2 and -2 can only be issued by gatekeepers, perhaps that should be explicit as well. +2 Approved for submission to repository by gatekeeper. -2 Will not be approved for submission to repository by gatekeeper. [07:49:25] I'm not sure if, in the long term, we want to codify that +2 and -2 can only come from gatekeepers. [07:49:33] Given that there is still the 'submit' step. [07:49:50] I don't think you should try to interpret the descriptions (not messages) we inherited as having anything to do with our workflow. [07:50:15] We're talking about changing the descriptions though. [07:50:40] In particular, because some people feel that the designations attached to the +1 and -1 scores are wrong. [07:51:34] +2 approved -2 rejected don't be excessively verbose "by gatekeeper" is superfluous. It doesn't help to say "submission... by gatekeeper", because only the gatekeepers _can_ push to the real repo. It also doesn't help to say "approved... by gatekeeper", because it's obvious who approved it; their name is next to the +2 [07:52:47] ... and I do think, if we're going to scale, then there's probably a future in which areas of the code attract subject experts who the gatekeepers permit to 'approve' code within those areas. They would be able to grant 'approved', but a gatekeeper would still have to hit the 'submit' button. [07:52:55] Yes, I know we're talking about changing the descriptions. But the question of whether the existing messages are directed to the gatekeepers or not is pointless, because it assumes they were written for a workflow with gatekeepers. [07:53:11] Which they obviously weren't :) [07:53:18] Yes, I see +2/-2 being expanded to a large group, eventually. [07:53:20] right. [07:54:20] While I'm at it, I think I'm going to change the 0s (for both) to be "Not verified", "Not reviewed". [07:55:21] yeah, i figured eventually we'd have tree "area experts" and i think i proposed that at some point [07:55:24] And, for the same reason, I think 'approved' and 'rejected' are too terse, because get, in emails "$verificationScore; $reviewScore" - so both have to indicate what they refer to, just for clarity. [07:57:44] Jeff, the reason I am proposing verbose descriptions is that the existing value when received by submitters in e-mail are confusing them. They do not know anything about our workflow. The messages have to indicate to some extents what that is. It is also not necessarily clear that the submitter of a patch is going to know anything about the roles that individuals hold. Therefore, that needs to be indicated as well. [07:58:39] They're being confused because the messages are confusing, not because they're not verbose. [07:58:43] descriptions [07:59:19] We can't embed role information, because who can issue something is separate from what the thing is. [08:00:13] it may simply be the case that gerrit doesn't do what we want it to at the moment and a feature request should be submitted. [08:00:34] There's a comment field. Use it. [08:00:39] +2 "approved for merge" +1 "looks good to me" 0 "no comment" -1 "I would prefer this not be merged in its current form" -2 "this will not be merged in its current form" verification: -1 "verification failed" 0 "not verified" +1 "verified" [08:00:54] And jhutz is today's winner. [08:01:38] I'm a little unsure on +1. A +1 should mean acceptance or a favorable review, without necessarily indicating active support for the idea. [08:01:48] GONE for now [08:02:27] I think the whole thing about gatekeepers is a red herring, anyway. When you evaluate review comments, what you care about is the individual's standing. It's not a case of gatekeepers having one weight, and everyone else has another. I'd take review comments from some contributors much more seriously than those from others, and I suspect everyone is in that boat. [08:19:47] --- mmeffie has become available [09:03:01] --- mmeffie has left [10:11:42] --- deason has become available [10:14:21] --- deason has left [10:33:00] another set of eyes on RT 125110 would be helpful. I've requested additional information but from what we already have it appears to be the case that it is not safe to empty the rx_call iovq queue without holding the call lock. [10:34:17] A panic condition has been triggered because of an attempt to free an already free packet that is being used as a data buffer for a packet that itself is already marked free. [10:37:03] --- dev-zero@jabber.org has left [10:39:38] this looks very similar to the problem i was hitting early last week. when that pops back up to the top of my queue, i can try the patch in it, but i'm dealing with a more pressing issue right now. [10:51:04] So, one thing our current workflow makes a pain is developing on top of a patch that's in gerrit, but not yet on master. [10:51:06] i was looking at 125110 earlier. did he provide the flags? [10:51:20] (if that patch isn't yours) [10:51:28] sure. you get to rebase once it's cherry-picked [10:52:07] the packet from which the data buffers are being freed is marked "free" [10:52:10] Well, no, the problem is, what if I'm doing work that depends on Simon's patch, such that I can't usefully do work without his patch. [10:52:15] Yes. [10:52:18] sure, but is that the *only* flag? [10:52:28] Is it possible for me to checkout something that corresponds to a patch that is in gerrit? [10:52:33] Yes. [10:52:35] derrick, yes. the only flag is "free" [10:52:36] you can do the work. cherry-pick the patch, or pull it [10:52:50] But what if I want to submit it before the underlying patch is on the mainline. [10:53:05] So, I can checkout your patch, and create a branch, and do work there. If you update, I get to rebase. When your patch is accepted, I get to rebase to master, which should be painless. [10:53:10] then you get to rebase after review [10:53:26] I can't submit, because I have a merge commit. [10:53:27] sucks but not the end of the world [10:53:49] I think what I have to do is git checkout -b refs/changes/ [10:53:50] you can submit a merge commit. we just won't accept it in that form [10:53:58] No, the problem is, again assuming I am doing work based on Simon's patch, that I _cannot_ submit before his patch is merged, because his patch doesn't appear on master, so we don't have a common ancestor on that branch [10:54:13] i stand by my position. [10:54:18] Oh, hm. [10:54:21] Well, the common master on the branch is the merge commit. [10:54:27] Which my submission will include. [10:54:50] common ancestor on the branch, even. [10:55:02] so you can submit. we can review. when it comes time to accept, we can say "fix it", abandon the merge commit, and move on [10:55:18] Well yeh. But I'd rather not have merge commits appear in gerrit at all. [10:55:53] --- abo has left [10:55:54] --- stevenjenkins has left [10:56:24] i'd like a pony [10:56:49] --- abo has become available [10:56:59] You can't always have what you want, Derrick. But this is about Simon _not_ having what he _doesn't_ wnat. they're not similar :-) [10:57:19] And I can fix my problem, but Derrick's still not getting a pony. [10:57:35] git checkout -b

is what you want. [10:57:53] Or to have a fetch that includes refs/changes [10:57:53] put it in the wiki? [10:58:15] I think we might need a FAQ, or advanced topics section in the wiki for this kind of thing. [10:58:28] advanced topics seems like trt [10:58:35] --- cclausen has left [10:58:39] hm. should gerrit link to the wiki page? [10:59:05] Perhaps, yes. [10:59:34] But lots of this stuff should go somewhere more obvious than the wiki, too. [11:00:09] well, i'd argue it should be linked, but stay in the wiki [11:00:49] Well, at some point, probably around when you get your pony, the whole of the OpenAFS web site should be something more editable. [11:00:57] Is Jake's project going anywhere? [11:01:06] jake is busy with GSoC [11:01:08] Jake is behind on gsock [11:01:15] so i suspect until he's done there, ... [11:01:38] I suspect we should split technology from content. [11:02:02] Our experience here is that doing both at the same time goes nowhere fast. [11:02:10] until we do it, making the information we have best available is better than ot [11:02:11] jake is focused on content [11:02:12] not [11:02:31] Jake is behind on GSoC, I plan to start web design once GSoC ends/GSoC project is satisfactorily finished [11:02:50] --- abo has left [11:02:52] --- stevenjenkins has become available [11:02:54] We've already talked about one technology transition, and Russ has done most of the work to move the web pages to git. [11:03:17] That's good. I wonder if we could do something like icky-wiki to make that easier for folks to edit. [11:03:29] --- abo has become available [11:05:08] I think Confluence is worth exploring. One of the edit modes is download the current page into MS Word, edit there, and write the content back to the wiki to be merged. [11:05:21] I dunno. What I do know is right now, we have sandbox and prod web pages in AFS, and the whole thing in git, and I think we talked about putting it in gerrit but don't know if that happened. If there are a small number of people who will be doing lots of web stuff, I'm happy to create additional *.sb.openafs.org virtual servers that point to sandboxes they put in AFS. I'll even provide volumes and GCO principals, if that makes things easier. But I suspect that's all somewhat interim. [11:05:31] It is in gerrit, yes. [11:06:02] I think the problem with all of the Atlassian tools is that they're not open source. That's a big sticking point for me, and for many others I suspect. [11:06:24] It isn't a sticking point for me. [11:06:27] My requirements for a content-management system: - It must be able to "publish" by exporting HTML that we can write into AFS and serve from whatever servers we have. - It must be possible to extract the content in editable form in case we ever decide to move to something else. [11:07:47] they build their products with open source and support open source communities. I write code that can be incorporated into closed source products and license it as such. I do not believe that all software must be open source. If I did I might be more inclined to back GPL licensing. [11:08:27] I don't consider open source to be an ideological sticking point, in this case, but I also won't argue with the people who do. I do think it's likely to be a pracical issue; all the tools we've ever had for this stuff have been things we've hacked on, and it seems somewhat unlikely we won't want to hack on the tools that manage our web pages. For example, what will we do about authentication? [11:09:52] Authentication is an issue. I'm not convinced OpenID is the solution, although it is an easy answer. [11:10:41] --- Russ has become available [11:11:45] I also think there's an issue with deploying an infrastructure that's based on Java solutions. It put me off gerrit too, at least initially. [11:12:08] I'm not, either, but I'm not convinced it's not. I think we need... - something that allows users to register and do password resets without involvement from any of us (openID gives us that) - the ability to have services where anyone who can authenticate can autocreate an account and start using it; RT should do this, so should gerrit - the ability to have services where ~anyone can "register" but can't do anything until an admin enables them. [11:12:27] Gerrit already does this. [11:12:47] does what? oh, lets people create an account and start using it? yes [11:12:48] The reason I'm worried about OpenID is that it puts our authentication eggs in somebody else's basket. [11:13:09] And the latter, to a small extent. It could completely do the latter if we configured it that way. [11:13:15] To the extent that you think we need an openid provider, or that you think all users should have to use our basket? [11:13:58] I'm primarily worried about people who have "Submit" access. In that case, the only protection against somebody pushing code into our tree is their chosen OpenID provider. [11:14:15] Oh, sure. The difference is really what bits a new user gets, if any, and whether the service lets you create "new users" at all. For example, we could probably trivially add openid auth to RT, which would deal with the authentication management issues, but a privileged person would still have to create the RT account and bind it to an openid [11:14:44] Well, we could require that people with certain bits use an approved openid provider. We could even say that ours is the only one. [11:15:07] I bet we could even build an openid provider backed by the GCO Kerberos database. [11:15:15] Informatics have one of them. [11:15:15] Which would be useful for other reasons. [11:15:23] Well not backed by GCO, but you get my drift. [11:15:25] Oh? Tell me ore? [11:15:39] I presume it can't do creation. [11:15:47] Our iFriend system can. [11:15:52] tell me more? [11:16:20] So, you create an account with iFriend, which gives you a Kerberos principal in IFRIEND.INF.ED.AC.UK, and then you can authenticate to our IdP and use that as your OpenID. [11:16:38] iFriend gives you a Kerberos principal in exchange for your email address. Not a bad trade. [11:17:16] Our IDP isn't currently a production service, but everything else is. [11:17:30] --- abo has left [11:18:13] --- abo has become available [11:18:42] If you want to try it out, go to https://id.not-a-service.inf.ed.ac.uk/ [11:19:37] is it worth trying to standardize the replication protocol vishal powar started working on last year? thoughts? [11:19:53] --- stevenjenkins has left [11:20:47] it is certainly worth documenting it [11:21:10] only if we think it's worth pursuing [11:21:15] otherwise, no, no it's not [11:21:23] at least in a universe with finite time [11:22:27] I don't know enough about the protocol he was working on to judge its merit. I suspect you'd have to document it before I could make that determination :) [11:26:43] --- stevenjenkins has become available [11:29:19] Oh, that sounds cool. I could certainly see us using something like that to authenticate various of our services, and to make it easy for people to get Kerberos principals so we can put them on AFS ACL's. [11:32:11] Yeh. We do that. [11:32:40] It's currently a kludge, in that they (the Visitor) has to go to a registration page to activate their account for AFS before they can stuck on ACLs. [11:32:53] But it means we can use filedrawers for secure file exchange with remote users. [11:33:35] The plan is that the iFriend database will feed prometheus, so that AFS pts IDs will be managed for visitors in the same way as they are for regular users. But we're not quite there yet. [11:57:28] --- dev-zero@jabber.org has become available [13:12:55] --- Russ has left: Disconnected [13:17:34] --- sxw+ has become available [13:31:56] --- Russ has become available [14:19:50] --- matt has become available [14:20:15] --- haba has become available [14:20:42] derrick: I'd be happy to read some vishal code [14:21:46] --- stevenjenkins has left [14:25:14] --- stevenjenkins has become available [14:30:40] If anyone's around, I'd welcome thoughts on http://gerrit.openafs.org/234 - I think this is a warning-reduction change that we should discuss, rather than just applying, as it does have implications ... [14:31:49] That's the kauth thing/ [14:31:51] ? [14:33:15] Yes. [14:33:52] Basically, it's reduce warnings by casting our different structures for DES key types into each other whenever we use them interchangably. [14:36:58] Well, it's a little messier than that. I see lots of casts to (unsigned char *) when calling things that take a des_cblock as an argument. [14:38:53] I think I'd prefer to do this right, which means eliminating any use of char arrays as function arguments, and probably means normalizing on one set of DES types instead of two. [14:38:58] --- dev-zero@jabber.org has left [14:39:29] des_cblock is unsigned char[8], so I suspect that the cast may be enough to make the compiler shut up. [14:40:26] A single set of DES types is hard, because some of them come from the xg files, which can't agree on a name. [14:40:39] And we've got way more than 2 :) [14:40:43] It probably is, but the point of eliminating warnings is to actually eliminate errors, rather than using casts to mask potential errors. Just casting every argument to des_fixup_key_parity() to unsigned char * will shut up the compiler, but won't detect when the argument actually has type unsigned char ** or the like. [14:40:58] Indeed. [14:41:20] We probably can't do much about the xg files, but we should be able to contain that. [14:41:41] Anyway, DES argument issues hurt my brain. [14:42:03] My preference, so far, has been to use static inline functions in preference to casts, to preserve some semblance of type safety. [15:40:35] has anyone ever used the data collected into the static array 'theLog' from src/vol/vnode.c for debugging? [15:41:36] vnode.c VNLog collects data into 'theLog' but as far as I can tell it can only be used by a debugger. VNLog is not type safe and generates lots of warnings. I would like to either remove it or replace its implementation. [15:49:53] It would be trivial to make it not varargs. [15:50:09] All of the callers seem to use exactly 4 parameters. [15:50:09] I want to make it not afs_int32 [15:50:22] What do you want to make it instead? [15:50:24] the warnings are from passing in pointers [15:51:43] if we believe intptr_t is portable, then that would be a good choice. [15:51:59] however, if the code isn't being used perhaps getting rid of it is preferable [16:05:09] intptr_t or uintptr_t is used in several locations in the tree but only on Windows and Solaris [16:10:35] intptr_t requires Autoconf magic to define it to unsigned long if it doesn't already exist, but otherwise is portable. [16:10:51] Er, uintptr_t. intptr_t obviously should be signed long. [16:14:29] I'm very aware that this is likely to start a war, but I'd welcome comments on http://www.dementia.org/twiki/bin/view/AFSLore/CodingStyle [16:14:50] Particularly, Russ, if you have a chance to look over and comment on the section about what header files can be safely included without guards. [16:16:12] #include <afsconfig.h> #include <afs/param.h> ? [16:17:26] Oh. It hates me. Hang on. [16:18:12] Fixed. [16:19:50] Must we have quite so many rules? [16:20:29] That's not many rules. [16:20:38] Try reading OpenBSD's style(9) [16:21:13] > the includes/ directoy. You mean the include/ directory (or src/include/). [16:21:33] Indeed I do. [16:21:38] > Finally come the header files from this module, which should be > referenced using " " Looks like a markup error. [16:22:08] Well, maybe not. It wants a trailing period anyway. [16:22:23] Most of these are expected. I am doubtful about the proposed reliance inline functions. [16:23:31] Doubtful in what way? That it will work? That it's a good idea? [16:25:04] I don't understand all the cases where you're mandating its use. There are legitimate uses for casts. [16:25:26] There are, but I would argue that they are few and far between. [16:25:52] It's far more maintainable to use an inline function that goes from type A to type B, then to blindly cast everything to type B. [16:26:40] > Do not use $< for non-pattern rules in any cross-platform directory Curious, does that break on some platforms? [16:27:01] Russ says "yes". We discussed it a few days ago. [16:28:59] Do all supported compilers even allow mandating this? If they do, it's still overreaching, I believe. In particular, I'm concerned about overemphasizing stylistic concerns over actual code intent and correctness. [16:29:16] Both are important. [16:29:27] One is -less important-. [16:30:03] Unreadable code is unmaintainable code. So, stylistic concerns are important in that regard. [16:30:14] Conventions help, because they mean that people know where to look. [16:30:44] For instance, if you know that grep ^function will always tell you where a function is defined, then that's a useful thing. The one place in the code that doesn't do that is a pain in the neck. [16:30:45] It's impossible to disagree--but that for that very reason it's essential to be judicious about rule-making. [16:31:09] There's very little in that file that isn't already in README.DEVEL at the top level. [16:31:18] Yet, I flagged one. [16:31:25] Much of the content is just stating the indent rules clearly. [16:31:55] But yes, there is new stuff. And it's stuff that I think is useful. It's stuff that I'd be inclined to comment upon in a code review, and that's why I think it's worth putting it all there. [16:32:15] That implies your views are universally held. Are they? [16:32:27] Read the beginning of the document. [16:32:44] I'm not saying that my views are universally held. I'm saying that this is a place to start a discussion from. [16:33:12] Right, I wan't saying your views weren't important--merely responding about them. [16:33:45] $< in non-pattern rules breaks with many non-GNU makes. [16:34:41] Commenting as I go: [16:34:54] Does the Windows portion of the code use // comments? (Is it all C++?) [16:35:44] Russ: Windows: it does, including much new code; I would find claiming the code to be C++ in general dubious [16:36:05] Not that I like // [16:36:13] Additional headers that can be included without guards: time.h, stdarg.h, stddef.h. [16:37:33] Is it currently the norm in openafs code to indent nested ifdefs? [16:37:44] It's becoming the norm. [16:37:46] Additional headers that can be included without guards on all UNIX platforms, but which I'm not sure about for Windows: fcntl.h. [16:37:49] It's certaily common to label them. [16:38:02] Additional headers that can be included for all UNIX but need to be wrapped for Windows: syslog.h [16:38:22] "It's becoming the norm"--but, by consensus, or? [16:38:34] Well, I make the patches, they get committed ... [16:39:20] You mean #elif, not #elsif, for preprocessor directives. [16:39:49] Heh. [16:39:54] Indeed I do. No CPP parser in twiki, sadly :) [16:40:09] Perhaps #elsif is becoming the norm, somewhere. [16:40:29] Can we please have the argument about eliminating character 0x09 from source files? [16:40:30] Also, I would be more explicit about the intending, because #define should also be indented. Something like: "all preprocessor directives inside #if or #ifdef should be nested by adding one space after the # and before the directive name per nesting level." [16:40:47] I wholeheartedly agree with adding indentation of nested preprocessor directives to the coding style. [16:40:48] #elsif isn't valid. [16:40:59] But Simon's been writing too much perl, I think [16:41:04] Oh yes. [16:42:03] for functions, we should say something about namespace for new functions. "New exported functions in libraries should begin with a common prefix of at least two characters identifying that library," maybe? [16:42:32] http://www.eyrie.org/~eagle/software/inn/docs/hacking.html may be useful as a reference here. That's the coding style guidelines for INN. [16:42:55] I think I'd prefer that we just required the library/module name as a prefix. [16:42:58] modules should have a common prefix for things they export [16:43:56] We should say explicitly that braces are required around the body of an if or else block if that block contains a nested if statement with an else. [16:44:01] In other words, something like: [16:44:04] if (condition) [16:44:08] if (other condition) [16:44:13] code; [16:44:26] can be okay, but anything more complicated needs braces. [16:44:42] Otherwise, you get GCC ambiguous else warnings. [16:44:57] Yeh, so it is covered by the 'new code must be warnings clean' bit. [16:45:07] True. But it may be worth saying explicitly as well. [16:45:12] I'll add it. [16:45:35] It is--I appreciate that rule, as stated. [16:45:38] I think we should except casting away const from the inline function requirement. [16:46:18] Some things that I'd like to see added, but which may be controversial: [16:46:44] Prefer if (p != NULL) to if (p) for checking whether a pointer is null to make the fact that it's a pointer more explicit. [16:47:20] Yes. OpenBSD in fact say never use true or false tests on anything that isn't a boolean. [16:47:21] Prefer if (*p != '\0') to if (*p) when explicitly checking a char for the nul byte. [16:47:30] In other words, generally use if (var) only if var is a boolean. [16:47:31] Right. [16:48:18] Use if (strcmp(foo, bar) == 0) or if (0 == strcmp(foo, bar)) as one prefers, but don't use if (!strcmp(foo, bar)), which is horribly counterintuitive. [16:48:52] No strcpy or strcat in new code. Use strlcpy, strlcat, or afs_snprintf as appropriate instead. [16:49:23] Never use strncpy unless you explicitly need the special strncpy semantics, and if you don't know what those are, never use it. Use strlcpy or memcpy instead. [16:51:09] Always use size_t for sizes of objects, not int or unsigned long, even if they're small objects. Exception: wire protocol objects should instead use types with explicit sizes, such as afs_int32. [16:51:37] Always use either size_t or ptrdiff_t for offsets into data structures or memory blocks, not int or long. [16:52:41] All new APIs that take buffers should also take the length of the buffer as an additional parameter. [16:53:16] I don't know if it's worth saying something explicit about not using static or global variables. [16:53:46] Oh, additional header that can always be included on UNIX but needs wrapping for Windows: unistd.h. [16:54:00] I thought I'd caught that. I'll add it. [16:54:07] All of this is going in, by the way. [16:54:25] cool, thank you! [16:54:30] I should really create an account on the wiki. :) [16:55:59] In INN, we wrap limits.h with an Autoconf test, but I'm not sure if that's required these days or not. No idea about that one on Windows. [16:56:10] Oh, ctype.h can be included unconditionally everywhere. [16:56:30] sys/uio.h can be used unconditionally on UNIX, but isn't available on Windows. Dunno if we use struct iovec anywhere in the tree. [16:57:59] We do use struct iovec, but I think only in the kernel. [16:58:02] BSDi requires netinet/in.h be included before arpa/inet.h. Dunno if we care. [17:00:26] Okay, that's everything we've discussed added to the document. [17:01:23] Hm, should we say to move assignments outside of conditionals? I prefer that coding style, but it's occasionally nice to make exceptions for some while loops. [17:01:44] In other words, prefer code = function(); if (code != 0) to if ((code = function()) != 0). [17:03:03] I don't have a strong view on that. I'm happy to add that as something to be fought over later, if you would like. [17:04:59] Yeah, let's do that and see what other people think. [17:05:31] I agree with you Russ, I find that easier to read [17:06:34] --- mdionne has become available [17:06:42] matt: To respond to earlier, I've been hearing some interesting stuff about how having a long and fairly detailed coding style guide is actually preferrable to having a shorter one with fewer rules. Definitely if we're actually enforcing or even modifying patches to comply with rules, we should write them down. I'm not sure on the argument of having fewer rules, but I know I find a consistent coding style way easier to read. [17:08:10] Oh, speaking of which, we don't in the current document say anything about spacing around parens and operators. I personally prefer always adding spaces around operators (+, -, >, <), no space between a function name and the opening paren of its arguments, space after but not before commas separating arguments, space between if, for, while, etc. and the paren, and space between the close paren and the opening brace. [17:08:36] I think the code generally, but not entirely, follows those conventions now. I know some of the new patches didn't have whitespace around some operators sometimes, so we're definitely not enforcing that one at the moment. [17:09:08] We don't currently add whitespace after open parens or before close parens. Some projects do. [17:09:16] I prefer not, but we might want to say explicitly. [17:09:46] Saying whether or not to use whitespace after a cast is probably too fiddly. [17:12:14] Okay. [17:12:24] I think I'm really done now. :) [17:12:32] "All new code must compile cleanly with..." should probably not mention specific compiler options, but something like ".. compile cleanly with the current default compile options" ? [17:12:35] We pretty much have a consistent coding style now, because way back when, somebody ran ident over the tree [17:12:47] Ah, yes, that's right. [17:12:52] Compile cleanly when configured with --enable-warnings ? [17:13:06] Yeah, that sounds good to me. [17:13:11] (or, shortly, not blow up when compiled with --enable-werror ) [17:13:45] There are a few more that hopefully we can eventually add (-Wwrite-strings would be nice, for instance, although it requires being const-clean). [17:14:10] -Wnested-externs is also useful. [17:14:13] I laugh in your general direction :) [17:14:38] We're probably not far from enabling -Wnested-externs now, and the things it catches are generally going to be stuff we want to get rid of. [17:14:40] We need to take rxgen apart and put it back together again before we're const clean. [17:15:06] There's no harm adding it to the enable-warnings list. [17:15:31] The standard set of warnings I use for my new code is -Wall -W -Wendif-labels -Wpointer-arith -Wbad-function-cast -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs [17:16:18] -W is too aggressive for us right now. -Wwrite-strings is infeasible at the moment. -Wmissing-prototypes is going to be a bunch of annoying makework for our current code base and probably isn't worth it. [17:16:50] -Wendif-labels is probably fine, and probably won't catch anything. [17:17:02] -Wpointer-arith is dubious on cost/benefit tradeoff for a large existing code base. [17:17:25] Ah, -Wendif-labels is on by default. [17:17:48] (-Wpointer-arith requires that you not do pointer arithmatic on void *'s.) [17:18:15] Marcus suggested -Wpointer-arith [17:18:51] * Russ likes it, but you do have to add local variables for void *'s that you're treating as char *'s. I'm not sure how annoying that will be. [17:18:53] I suspect we're probably not that bad for it. Most of our codebase predates void *, and where I have been changing char * to void *, I haven't done so in places where we're doing pointer arithmetic. [17:19:22] -W enables -Wtype-limits, which is an absolute horrifiic pain for an existing code base. [17:19:28] Ouch. [17:19:30] * Russ went through that with INN, but it would be a nightmare for OpenAFS. [17:19:43] --- abo has left [17:19:44] --- stevenjenkins has left [17:19:47] -Wbad-function-cast ? [17:19:50] It requires that you not compare signed with unsigned ints. [17:19:56] -Wtype-limits [17:20:06] -Wbad-function-cast we can use. [17:20:21] --- abo has become available [17:20:24] It warns about casts of function pointers to anything other than the actual type of the function. [17:20:30] I suspect signed vs unsigned is probably something we should tackle in the long run. [17:20:31] I suspect we would already pass it. [17:20:39] Probably, yes. [17:20:41] * Russ fixed some real bugs in INN by using -W. [17:20:53] * Russ also introduced a bunch of bugs by fixing signed/unsigned comparisons incorrectly. [17:21:00] It's not something to tackle lightly. [17:21:07] Yeh. If volume IDs, for instance, approach the point where top bit is set, then signed vs unsigned is going to become important. [17:21:08] The fix often requires serious thought. [17:21:39] Let's not do it right now, but I think it's worth considering. [17:21:43] It's also like making your code const-clean: it uncovers lots of places where your data structures and function signatures were simply wrong, and tells you to fix it. [17:22:05] Which can be daunting and require cascading work through lots of code. [17:22:17] My rough plan is that once the tree is 'fairly' clean, all of the stuff that gets turned on with --enable-warnings will get switched on by default. And --enable-warnings will then have a more agressive set of options. [17:22:43] Yeah, that sounds good. [17:22:49] --enable-werror, or --enable-checking, or whatever we call it, would use the lesser set of options, but the build would fail on new errors. [17:23:01] I'm guessing the right approach will be to not just add -W (-Wextra these days) and instead slowly add the specific components of it. [17:23:12] Probably, yes. [17:23:12] Some of them are much easier than others, such as -Wclobbered. [17:23:32] Which checks safe longjmp handling. [17:23:42] Although I suspect we then get into a fun "does gcc support ..." problem. [17:23:48] Would lwp pass it? [17:23:54] Not sure. Good question. [17:24:05] But at least code that doesn't is generally fairly self-contained. [17:24:48] Oh, -Wempty-body reminds me: we should probably say in the coding style that one should always write: for (...; ...; ...) ; [17:24:52] instead of: [17:24:57] for (...; ...; ...); [17:25:13] to make the empty loop body explicit. [17:25:21] Okay. [17:25:33] * sxw+ doesn't have -Wempty-body on this gcc [17:26:09] --- stevenjenkins has become available [17:26:22] --- abo has left [17:26:51] --- abo has become available [17:27:33] Yeah, it's probably only available in -W. [17:27:41] I forget when gcc made sure that all -W warnings had their own flags. [17:27:44] It's relatively recent. [17:28:24] The other thing that you'll run into with different gcc versions is that it's not until fairly recently (at least 4.x era) that gcc got intelligent about not warning about constructs in system headers. Older versions will warn about bugs in system headers that you can't do anything about. [17:28:54] gcc still can't cope with bugs introduced in preprocessor macros that are in system headers. [17:29:14] I have to suppress some warnings when building Perl extensions, for instance, because perl.h has macros that aren't warning-clean for the warnings I use. [17:29:17] Yeh. I suspect that the number of platforms we can meaningfully run with -Werror will be small [17:29:49] And the pragma stuff to disable warnings doesn't work with that many warning types, sadly. [17:30:11] I think that gets better the more recent GCC you have. That's a very new feature. [17:30:29] It was only added after GCC itself started using -Werror, which I think is within the past year or two. [17:33:20] But now, I must sleep. Anyone else with wiki permissions - feel free to edit the CodingStyle document. Night all! [17:36:05] So, wiki permissions comes from? [17:38:41] Derrick, I assume. [17:38:55] yes [17:39:11] you can create an account on your own, but he has to give you edit permission [17:41:33] Why not encourage folks to...get them? [17:42:33] them being accounts or write permission? [17:43:05] The reason to have a Wiki is for folks to collaborate in writup, I would have thought. So. [17:43:44] yeah, I'm not sure what the reasoning is. But I tend to agree, no reason people shouldn've have them (not my server though). [17:45:58] The only reason why accounts have to be approved first is because we were getting buried in wiki spam. [17:46:09] I didn't think it was that hard to get them. It is not unreasonble to require approval to edit. [17:46:27] I suspect the only reason why the wiki isn't being advertised is because Derrick is running it on a spare machine somewhere and no one has had much time to make a big deal of it. [17:46:29] oh, it's not hard to get them, just talk to Derrick. [17:47:18] or, wasn't a year ago, but I assume he hasn't changed his policy [17:48:42] A page explaining how to sign up might make a nice addition to the Wiki. [17:50:01] http://www.dementia.org/twiki/bin/view/TWiki/TWikiRegistration [17:50:33] then I think you just ping derrick [18:10:39] --- Russ has left: Disconnected [18:15:15] --- matt has left [18:27:46] --- Russ has become available [19:32:25] --- mdionne has left [19:43:20] --- Russ has left: Disconnected [20:34:58] "explaining how to sign up for the wiki": if you can't find the register link on every page, me telling you to click on it won't help [20:36:13] any wiki editer should be able to add more, actually, but when i get email notification someone signed up and it's an actual, recognizable person i generally just give it without them asking [21:03:32] the vishal code is in RT, btw [21:06:21] --- asedeno has left [21:06:22] --- asedeno has become available [21:11:42] --- dev-zero@jabber.org has become available [23:04:20] --- dev-zero@jabber.org has left: Replaced by new connection [23:04:21] --- dev-zero@jabber.org has become available