* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code @ 2012-04-24 17:37 Drew Adams 2012-04-24 17:49 ` Drew Adams ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Drew Adams @ 2012-04-24 17:37 UTC (permalink / raw) To: 11328 Just a nit. But if you are going to add unnecessary comments to the code that describe only what everyone can see the code does, then at least get them right. Otherwise you mislead readers. This comment is incorrect: "Not a directory". What is actually true at that point is the following; a. RECURSIVE is nil b. RECURSIVE is non-nil and this is not a directory c. this is a directory, RECURSIVE is non-nil and not `always', and the user replied `n' (Similarly, the comment "This is a directory", though true, does not convey the real meaning. It is a directory AND it should be copied recursively.) It is a bad habit to add such comments to the code. Comments should generally be used when it is not obvious what the code does or why. In GNU Emacs 24.1.50.1 (i386-mingw-nt5.1.2600) of 2012-04-23 on MARVIN Bzr revision: 108006 agustin.martin@hispalinux.es-20120423103325-xmra3329elgzhmpc Windowing system distributor `Microsoft Corp.', version 5.1.2600 Configured using: `configure --with-gcc (4.6) --no-opt --enable-checking --cflags -ID:/devel/emacs/libs/libXpm-3.5.8/include -ID:/devel/emacs/libs/libXpm-3.5.8/src -ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include -ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include -ID:/devel/emacs/libs/giflib-4.1.4-1/include -ID:/devel/emacs/libs/jpeg-6b-4/include -ID:/devel/emacs/libs/tiff-3.8.2-1/include -ID:/devel/emacs/libs/gnutls-3.0.9/include -ID:/devel/emacs/libs/libiconv-1.13.1-1-dev/include -ID:/devel/emacs/libs/libxml2-2.7.8/include/libxml2' ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-24 17:37 bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code Drew Adams @ 2012-04-24 17:49 ` Drew Adams 2012-04-24 18:22 ` Drew Adams 2012-04-24 18:54 ` Stefan Monnier 2014-02-09 4:32 ` Lars Ingebrigtsen 2 siblings, 1 reply; 13+ messages in thread From: Drew Adams @ 2012-04-24 17:49 UTC (permalink / raw) To: 11328 Oh, and since there is no doc string it might be a good idea to use better parameter names. There is a reason that for `copy-file' and `make-symbolic-link' the parameter is called OK-IF-ALREADY-EXISTS and not simply OK-FLAG. (Same problem for `dired-copy-file'.) You can guess the meaning from OK-IF-ALREADY-EXISTS. You are not helping readers of the code if they need to examine it all closely to determine what a given parameter does. In that case, you might as well name the parameters X1, X2, X3, X4, X5, and X6. And it's generally a good idea to use the same parameter names when you just pass the arguments to another function and they have the same meaning. If you just pass PRESERVE-TIME to `copy-directory' and `copy-file', then use the same name they use: KEEP-TIME. (Or change all three and any other functions related so they use the same names.) ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-24 17:49 ` Drew Adams @ 2012-04-24 18:22 ` Drew Adams 2012-04-25 13:40 ` Nix 0 siblings, 1 reply; 13+ messages in thread From: Drew Adams @ 2012-04-24 18:22 UTC (permalink / raw) To: 11328 While on the subject of comments in this file, you might want to change "fluid variable" everywhere to "free variable". This is perhaps a translation problem. (AFAIK, in English fluid variables have only to do with fluid mechanics.) ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-24 18:22 ` Drew Adams @ 2012-04-25 13:40 ` Nix 2012-04-25 16:26 ` Drew Adams 0 siblings, 1 reply; 13+ messages in thread From: Nix @ 2012-04-25 13:40 UTC (permalink / raw) To: Drew Adams; +Cc: 11328 On 24 Apr 2012, Drew Adams stated: > While on the subject of comments in this file, you might want to change "fluid > variable" everywhere to "free variable". This is perhaps a translation problem. > (AFAIK, in English fluid variables have only to do with fluid mechanics.) Fluid variables are a Scheme thing (more, generally, a functional-programming thing). "Dynamically-scoped free variable" would be a reasonable thing to replace it with. If this were a docstring or a string in the manual, I'd agree that either this needs replacing, or fluids need a definition -- but this is a code comment, and finding out what a fluid is once you see that is a matter of ten seconds looking in, say, the Guile manual. -- NULL && (void) ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-25 13:40 ` Nix @ 2012-04-25 16:26 ` Drew Adams 2012-04-25 18:42 ` Thierry Volpiatto 0 siblings, 1 reply; 13+ messages in thread From: Drew Adams @ 2012-04-25 16:26 UTC (permalink / raw) To: 'Nix'; +Cc: 11328 > Fluid variables are a Scheme thing (more, generally, a > functional-programming thing). "Dynamically-scoped free > variable" would be a reasonable thing to replace it with. Bof! Googling just a bit suggests that a fluid variable is more or less a dynamically scoped variable. That does not at all make it a _free_ variable. The variable occurrences noted in this code were free - no matter how scoped. Free in lambda calculi, free if dynamically scoped, free if lexically scoped. Free variables. And that is presumably why the comments were inserted: to point out to readers that the occurrences are not bound locally. > finding out what a fluid is once you see that is a matter of > ten seconds looking in, say, the Guile manual. No, it is not a matter of ten seconds. And the term "fluid variable" does not even appear in the Guile manual. The closest the manual comes, AFAICT, is this: "Fluids can also be used to simulate the desirable effects of dynamically scoped variables." NB: "also" and "simulate" and only certain "effects of". IOW, the Guile manual does not even say that a fluid is a dynamically scoped variable, let alone a d-s _free_ variable. A fluid is said to be an object that can store one value per dynamic state. The claim wrt dynamic scoping is only that, among its various uses, a fluid can simulate some of the effects of a dynamically scoped variable. (The Guile manual, BTW, distinguishes fluid vars from "parameters", which it says are "like dynamically bound variables in other Lisp dialects". The distinction being apparently that parameters are per-thread.) Anyway, this is not Guile - or Scheme of any sort. In the same vein, here is a quote from Emacs Wiki, presumably written by a Schemite. It indicates a similar parochialism: "Scheme was the first language to introduce lexical binding. ... Variables subject to dynamic binding are usually referred to as `fluid variables' or `parameters' on these systems." (http://www.emacswiki.org/emacs/DynamicBindingVsLexicalBinding#Scheme) In the Beginning there was The Scheme... First to introduce lexical binding, indeed! But even if dynamically bound variables are "usually" referred to as "fluid variables" among Scheme enthusiasts, that is no reason to suppose that Computer Science or programming practice in general refers to them that way. And anyway, the code comments I commented on are about _free_ variable occurrences. They do not simply point out variables that are dynamically scoped. FWIW, the `librep' manual says that "fluid variables" aka fluids are another "method of implementing dynamically scoped variables." The sole purpose of such an object, it claims, is "to provide a location from which dynamic bindings may be created." (And yet librep's claim to fame is that it has improved on Emacs Lisp in this way: It "was originally inspired by Emacs Lisp. However one of the main deficiencies of elisp--the reliance on dynamic scope--has been removed." Removed ... and then implemented, it seems, using fluids.) FWIW, here's a Joulist characterization of fluid variables, which claims that Scheme has _no_ fluid variables: "A fluid variable is a symbol and cell that is established as part of the current environment. When a fluid variable (symbol) is referenced the current nest of environments is searched for a fluid variable with the matching symbol. The first such match provides the cell that satisfies the reference. This really means that each continuation embodies a set of fluid bindings. Many uses of continuations, for which they were reified, are incompatible with fluid variables. One such use is time-slicing. Scheme has no fluid variables." http://www.cap-lore.com/Languages/Global.html The same article mentions global vars, static class vars, "fluid variables in Common Lisp", and dynamically scoped vars as being "related". I personally have not seen "fluid variable" used wrt Common Lisp, which AFAIK calls it a "special variable". In short, I do not see where, even in the limited world of Scheme or Guile, a free variable occurrence is a "fluid variable" or vice versa. No, I am no expert on SkemeSpeke. But I can usually recognize a free variable when I see one. Mea culpa: I googled for "fluid variable" before filing the bug, and it seemed that the term was essentially limited to fluid mechanics. So I guessed (wrongly) that its use here might have been a mistranslation from some other natural language to English. I did not guess that it was simply the _use_ of another language, unnatural, and not a translation at all. So let me translate my bug report: time to translate to English. These are _free_ variable occurrences. ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-25 16:26 ` Drew Adams @ 2012-04-25 18:42 ` Thierry Volpiatto 2012-04-25 21:51 ` Drew Adams 0 siblings, 1 reply; 13+ messages in thread From: Thierry Volpiatto @ 2012-04-25 18:42 UTC (permalink / raw) To: 11328 Hi Drew, about fluid variables in dired code, particularly in `dired-create-files', I would say instead of fluid "obscure", as they are variables that must be in any calls of `dired-create-files' within a lambda used as argument of this function. I think it would be better to have explicit arg in the caller and have also the lambda's there. The code would be easier to read and understand and no need to have obscure comments such as "fluid variable...etc". -- Thierry Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997 ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-25 18:42 ` Thierry Volpiatto @ 2012-04-25 21:51 ` Drew Adams 2012-04-26 5:48 ` Thierry Volpiatto 0 siblings, 1 reply; 13+ messages in thread From: Drew Adams @ 2012-04-25 21:51 UTC (permalink / raw) To: 'Thierry Volpiatto', 11328 Hi Thierry, > about fluid variables in dired code, particularly in > `dired-create-files', I would say instead of fluid "obscure", as they > are variables that must be in any calls of `dired-create-files' > within a lambda used as argument of this function. > I think it would be better to have explicit arg in the caller and have > also the lambda's there. The code would be easier to read and > understand and no need to have obscure comments such as "fluid > variable...etc". That the use of those vars can make the code obscure is one thing. But what makes the vars potentially problematic is that they are free. If there is to be a comment in the code to draw attention to this then the comment should say that the vars are _free_ here or there. A comment should not just say that they are "obscure" (I know you did not suggest that). What you suggest is one approach to eliminating the free occurrences. I'm not sure that's really needed or is the best approach. I have no opinion about that - I don't really care much one way or the other. To know what the best approach is someone would need to spend a bit of time with the code. There are also some other approaches, if we did want to eliminate those free occurrences: The code (e.g. callers) could just use either (a) lexical scoping (`lexical-let' or file-level) to capture the variable plus its value within the lambda closure, or (b) backquote with quote+comma to capture only the value (i.e., a pseudo-closure: no var at all, just the value). E.g., in the NAME-CONSTRUCTOR arg that is passed by `dired-do-create-files' to `dired-create-files', the code could use this, substituting TARGET's value instead of leaving TARGET as a free var in the lambda: `(lambda (from) (expand-file-name (file-name-nondirectory from) ',target)) instead of: (lambda (from) (expand-file-name (file-name-nondirectory from) target)) Or it could just use the latter if TARGET were lexically bound with the right value. In that case the lambda would form a closure. That's an easy one. There is also the more convoluted case of `d-do-copy', which calls `d-create-files', which binds `d-overwrite-confirmed' around its funcall of the FILE-CREATOR arg, which is `d-copy-file' in this case, which calls `d-handle-overwrite' (without passing `d-overwrite-confirmed'), which uses `d-overwrite-confirmed'. Maybe that's what you had in mind. First thing about that one is that the funcall actually passes `d-overwrite-confirmed' as an arg to `d-copy-file', in addition to binding it for use by `d-handle-overwrite'. It would be simpler to just add it as a parameter for `d-handle-overwrite' and then let `d-copy-file' and others pass it along explicitly to that function. Second thing is that the value of `overwrite-backup-query', which var is free in `d-handle-overwrite', is never even used anywhere. That var is bound in `d-create-files' presumably only because `d-query', to which it is passed, expects a variable (which it sets - in this case uselessly). There is plenty of such convoluted stuff in the Dired code. No doubt some of it could be simplified, but the cleanup would have to be careful and be sure not to change any behavior. And some changes will likely affect 3rd-party code (e.g. Dired+). One reason for such complicated code (a guess) is that someone at one point tried to define some generic code (macros, functions), and other code was written to use that code, and then later someone needed slightly different generic functions (e.g. additional parameters), but instead of modifying the generic functions (because there was already consuming code that expected the existing interfaces) they fudged other ways of getting the additional info to the recipients. Such is life. Other than that, the same ideas above apply. Suppose that there is some good reason not to add a parameter to `d-handle-overwrite' in order to pass the `d-overwrite-confirmed' value. Or suppose that `d-copy-file' did not accept a third arg, and that it were only `d-handle-overwrite' that needed the `d-overwrite-confirmed' value. In such cases, to get the `d-overwrite-confirmed' value to the innards of such functions that do not accept it as an arg, instead of funcalling FILE-CREATOR directly, `d-create-files' could funcall a lambda that encapsulates the value of `d-overwrite-confirmed' and calls FILE-CREATOR. That too would explicitly reflect the fact that the file creator function depends on that value. Or if `d-copy-file' & compagnie are used only for `d-create-files', then put their defuns inside it lexically (nested defuns) and use lexical scoping for the file. And so on. There are different ways to eliminate the free vars or wrap them together with their values in a closure. And perhaps the code could anyway be simplified in other ways, which might obviate any such need. Dunno. I haven't bothered to look closely at it (I don't care enough). Again, if someone does that, I really hope they are careful. Or we can just live with the free vars, in which case a comment doesn't hurt. But it should say "free", not "fluid", IMO. ;-) - Drew ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-25 21:51 ` Drew Adams @ 2012-04-26 5:48 ` Thierry Volpiatto 2012-04-26 14:09 ` Drew Adams 2012-04-26 15:35 ` Drew Adams 0 siblings, 2 replies; 13+ messages in thread From: Thierry Volpiatto @ 2012-04-26 5:48 UTC (permalink / raw) To: Drew Adams; +Cc: 11328 "Drew Adams" <drew.adams@oracle.com> writes: > What you suggest is one approach to eliminating the free occurrences. I'm not > sure that's really needed or is the best approach. I have no opinion about that > - I don't really care much one way or the other. Yes me too now, but if I remember the first time I use `dired-create-files' in my code, it took me some time to figure how to use this. This is why I say it would be nice to clarify the use of `dired-create-files'. Just take example of TARGET, that could be an argument of `dired-create-files' instead of using NAME-CONSTRUCTOR. Actually you must give TARGET to d-c-files within a lambda (NAME-CONSTRUCTOR): --8<---------------cut here---------------start------------->8--- (dired-create-files fn ; the file-creator a function e.g `dired-copy-file' (symbol-name action) files ; A list of files to apply file-creator on. (if (file-directory-p target) ; A lambda form that handle the special arg TARGET. #'(lambda (from) (expand-file-name (file-name-nondirectory from) target)) #'(lambda (from) target)) marker) --8<---------------cut here---------------end--------------->8--- It would be more clear to call d-c-files like this: --8<---------------cut here---------------start------------->8--- (dired-create-files fn ; the file-creator a function e.g `dired-copy-file' (symbol-name action) files ; A list of files to apply file-creator on. ;; The `if' form above containing the lambda is now in `dired-create-files' ;; give it TARGET to handle. target marker) --8<---------------cut here---------------end--------------->8--- Of course using a lambda as arg like it is done actually seem more flexible, but I don't think it could be used in many differents way. i.e in others way than the one in `dired-do-create-files'. > To know what the best approach is someone would need to spend a bit of > time with the code. > There are also some other approaches, if we did want to eliminate those free > occurrences: > > The code (e.g. callers) could just use either (a) lexical scoping (`lexical-let' > or file-level) to capture the variable plus its value within the lambda closure, > or (b) backquote with quote+comma to capture only the value (i.e., a > pseudo-closure: no var at all, just the value). > > E.g., in the NAME-CONSTRUCTOR arg that is passed by `dired-do-create-files' to > `dired-create-files', the code could use this, substituting TARGET's value > instead of leaving TARGET as a free var in the lambda: > > `(lambda (from) > (expand-file-name (file-name-nondirectory from) ',target)) This would be even more complex to understand how to use d-c-files. > instead of: > > (lambda (from) > (expand-file-name (file-name-nondirectory from) target)) > > Or it could just use the latter if TARGET were lexically bound with the right > value. In that case the lambda would form a closure. > > That's an easy one. There is also the more convoluted case of `d-do-copy', > which calls `d-create-files', which binds `d-overwrite-confirmed' around its > funcall of the FILE-CREATOR arg, which is `d-copy-file' in this case, which > calls `d-handle-overwrite' (without passing `d-overwrite-confirmed'), which uses > `d-overwrite-confirmed'. Maybe that's what you had in mind. > > First thing about that one is that the funcall actually passes > `d-overwrite-confirmed' as an arg to `d-copy-file', in addition to binding it > for use by `d-handle-overwrite'. It would be simpler to just add it as a > parameter for `d-handle-overwrite' and then let `d-copy-file' and others pass it > along explicitly to that function. > > Second thing is that the value of `overwrite-backup-query', which var is free in > `d-handle-overwrite', is never even used anywhere. That var is bound in > `d-create-files' presumably only because `d-query', to which it is passed, > expects a variable (which it sets - in this case uselessly). > > There is plenty of such convoluted stuff in the Dired code. No doubt some of it > could be simplified, but the cleanup would have to be careful and be sure not to > change any behavior. And some changes will likely affect 3rd-party code (e.g. > Dired+). Same here (helm), but this would not be difficult to fix. > There are different ways to eliminate the free vars or wrap them together with > their values in a closure. And perhaps the code could anyway be simplified in > other ways, which might obviate any such need. Dunno. I haven't bothered to > look closely at it (I don't care enough). Again, if someone does that, I really > hope they are careful. Agree, it is the problem with such obscure code, unexpected behavior, but in this case I don't think it is too scary to simplify it. > Or we can just live with the free vars, in which case a comment doesn't hurt. > But it should say "free", not "fluid", IMO. ;-) Never understood what "fluid" mean, but we can't say also they are really "free" (even if they are sort of free) because they are (will be) all let-bounded in some places. i.e they just not figure in the code but they must be apported latter by the caller. -- Thierry Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997 ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-26 5:48 ` Thierry Volpiatto @ 2012-04-26 14:09 ` Drew Adams 2012-04-26 15:35 ` Drew Adams 1 sibling, 0 replies; 13+ messages in thread From: Drew Adams @ 2012-04-26 14:09 UTC (permalink / raw) To: 'Thierry Volpiatto'; +Cc: 11328 > we can't say also they are really "free" (even if they > are sort of free) because they are (will be) all > let-bounded in some places. Look up the definition of free variable - see the lambda calculus. It is always about free _occurrences_ of a variable. Freedom of a variable (not a great word for it, admittedly) is relative to a given context. Whether a variable is bound in some outer lambda is not the question. What matters is whether it is free in some other (e.g. inner) lambda. X is a free variable in the inner lambda form here, though it is bound in the outer one, i.e., in the overall expression. (funcall (funcall (lambda (x) (lambda (y) (+ x y))) 42) ; Fn that increments by 42. 3) ; Applied to 3 gives 45. That is equivalent to this: (let ((x 42)) (funcall (lambda (y) (+ x y)) 3) (It's perhaps clearer if you remove all the `funcall' atoms. That will give you Lisp 1 syntax ~= lambda calculus syntax.) ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-26 5:48 ` Thierry Volpiatto 2012-04-26 14:09 ` Drew Adams @ 2012-04-26 15:35 ` Drew Adams 2012-04-26 18:38 ` Thierry Volpiatto 1 sibling, 1 reply; 13+ messages in thread From: Drew Adams @ 2012-04-26 15:35 UTC (permalink / raw) To: 'Thierry Volpiatto'; +Cc: 11328 > Just take example of TARGET, that could be an argument of > `dired-create-files' instead of using NAME-CONSTRUCTOR. > [Currently] you must give TARGET to d-c-files within a lambda > (NAME-CONSTRUCTOR) > It would be more clear to call d-c-files like this: > > (dired-create-files > fn > (symbol-name action) > files > ;; The `if' form above containing the lambda > ;; is now in `dired-create-files' > target ; Give it TARGET to handle. > marker) As I explained, in the particular case of NAME-CONSTRUCTOR and TARGET the caller does not in fact ever need (make use of) the _variable_ TARGET. All it needs is its value, i.e., the value at the time and place that the lambda is constructed, in `d-do-create-files'. So there is no need to include the _variable_ itself in the lambda form. It is sufficient to use its value there. That is clearer to read and is cleaner code. And there is no need either to pass TARGET as an additional argument to `d-create-files'. > > E.g., in the NAME-CONSTRUCTOR arg that is passed by > > `dired-do-create-files' to `dired-create-files', the code > > could use this, substituting TARGET's value > > instead of leaving TARGET as a free var in the lambda: > > > > `(lambda (from) > > (expand-file-name (file-name-nondirectory from) ',target)) > > > > instead of: > > > > (lambda (from) > > (expand-file-name (file-name-nondirectory from) target)) > > This would be even more complex to understand how to use d-c-files. I don't think so. It has nothing to do with how to use `d-c-files'. Do you find this clearer? (lambda (from) (expand-file-name (file-name-nondirectory from) "/foo/bar")) I assume so. No TARGET variable there. I've just substituted its current value at the time the lambda form was constructed (i.e., in `d-do-create-files') - let's assume "/foo/bar" in this call to `d-do-create-files'. How about this? (list 'lambda (list 'from) (list 'expand-file-name (list 'file-name-nondirectory 'from)) (symbol-value 'target)) Those three are all the same thing (assuming TARGET is "/foo/bar" in `d-do-create-files'). The point is that the lambda form need not contain the (free) variable TARGET at all. It is enough that it use the variable's _value_. And Occam's razor says that if it need not then it should not - just get the value at lambda construction time/place and plug it in. The caller of the lambda need never bother with the variable at all, in any context. Again, however, this is the simple case - NAME-CONSTRUCTOR. In other cases the caller does in fact use the variable itself, looking it up in some particular context to get its value there, or perhaps even setting it. But in this simple case the caller does not need the variable - its value at the time and place of the lambda construction is all that is needed. And the code is clearer if we make that explicit. No sense letting a reader mistakenly think that the caller might somehow use the variable TARGET. In fact, it takes a bit of looking at the code to realize this. Far better to make it clear to readers from the outset. > > Or it could just use the latter if TARGET were lexically > > bound with the right value. In that case the lambda would form a > > closure. In that case, we would be encapsulating the variable's binding at the lambda construction place (not time, however, since binding is lexical). That's overkill, but it amounts to the same thing. The only difference is that the variable value would be looked up when the lambda is _used_, not when it was constructed. But that use-time lookup just returns the value that was in play at the time of the lambda construction. So same effect in the end. HTH - Drew ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-26 15:35 ` Drew Adams @ 2012-04-26 18:38 ` Thierry Volpiatto 0 siblings, 0 replies; 13+ messages in thread From: Thierry Volpiatto @ 2012-04-26 18:38 UTC (permalink / raw) To: Drew Adams; +Cc: 11328 "Drew Adams" <drew.adams@oracle.com> writes: > As I explained, in the particular case of NAME-CONSTRUCTOR and TARGET the caller > does not in fact ever need (make use of) the _variable_ TARGET. All it needs is > its value, i.e., the value at the time and place that the lambda is constructed, > in `d-do-create-files'. > > So there is no need to include the _variable_ itself in the lambda form. It is > sufficient to use its value there. That is clearer to read and is cleaner code. Yes that is what i wanted to explain. > And there is no need either to pass TARGET as an additional argument to > `d-create-files'. Of course there is no need to do this, it would change nothing, just make the code clearer to use and read. What can be done also is leave `dired-create-files' as it is and modify `dired-do-create-files' to have a generic function usable anywhere (actually it is usable only in dired) and easy to use; It is what I do here. -- Thierry Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997 ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-24 17:37 bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code Drew Adams 2012-04-24 17:49 ` Drew Adams @ 2012-04-24 18:54 ` Stefan Monnier 2014-02-09 4:32 ` Lars Ingebrigtsen 2 siblings, 0 replies; 13+ messages in thread From: Stefan Monnier @ 2012-04-24 18:54 UTC (permalink / raw) To: Drew Adams; +Cc: 11328 I really appreciate your nit-picks (I know the word sounds negative, but to improve code-quality, it's really what is needed). But please, please, pretty please, get yourself access to the Bzr so you can fix those things yourself. You'll be happier, we'll be happier, the world will smile, birds will sing. Stefan >>>>> "Drew" == Drew Adams <drew.adams@oracle.com> writes: > Just a nit. But if you are going to add unnecessary comments to the > code that describe only what everyone can see the code does, then at > least get them right. Otherwise you mislead readers. > This comment is incorrect: "Not a directory". What is actually true at > that point is the following; > a. RECURSIVE is nil > b. RECURSIVE is non-nil and this is not a directory > c. this is a directory, RECURSIVE is non-nil and not `always', > and the user replied `n' > (Similarly, the comment "This is a directory", though true, does not > convey the real meaning. It is a directory AND it should be copied > recursively.) > It is a bad habit to add such comments to the code. Comments should > generally be used when it is not obvious what the code does or why. > In GNU Emacs 24.1.50.1 (i386-mingw-nt5.1.2600) > of 2012-04-23 on MARVIN > Bzr revision: 108006 > agustin.martin@hispalinux.es-20120423103325-xmra3329elgzhmpc > Windowing system distributor `Microsoft Corp.', version 5.1.2600 > Configured using: > `configure --with-gcc (4.6) --no-opt --enable-checking --cflags > -ID:/devel/emacs/libs/libXpm-3.5.8/include > -ID:/devel/emacs/libs/libXpm-3.5.8/src > -ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include > -ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include > -ID:/devel/emacs/libs/giflib-4.1.4-1/include > -ID:/devel/emacs/libs/jpeg-6b-4/include > -ID:/devel/emacs/libs/tiff-3.8.2-1/include > -ID:/devel/emacs/libs/gnutls-3.0.9/include > -ID:/devel/emacs/libs/libiconv-1.13.1-1-dev/include > -ID:/devel/emacs/libs/libxml2-2.7.8/include/libxml2' ^ permalink raw reply [flat|nested] 13+ messages in thread
* bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code 2012-04-24 17:37 bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code Drew Adams 2012-04-24 17:49 ` Drew Adams 2012-04-24 18:54 ` Stefan Monnier @ 2014-02-09 4:32 ` Lars Ingebrigtsen 2 siblings, 0 replies; 13+ messages in thread From: Lars Ingebrigtsen @ 2014-02-09 4:32 UTC (permalink / raw) To: Drew Adams; +Cc: 11328 "Drew Adams" <drew.adams@oracle.com> writes: > Just a nit. But if you are going to add unnecessary comments to the > code that describe only what everyone can see the code does, then at > least get them right. Otherwise you mislead readers. > > This comment is incorrect: "Not a directory". What is actually true at > that point is the following; Fixed on trunk. As for the "fluid variable" thing, I think that's probably left as is, or the code should be rewritten. Closing. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog http://lars.ingebrigtsen.no/ ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-02-09 4:32 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-24 17:37 bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code Drew Adams 2012-04-24 17:49 ` Drew Adams 2012-04-24 18:22 ` Drew Adams 2012-04-25 13:40 ` Nix 2012-04-25 16:26 ` Drew Adams 2012-04-25 18:42 ` Thierry Volpiatto 2012-04-25 21:51 ` Drew Adams 2012-04-26 5:48 ` Thierry Volpiatto 2012-04-26 14:09 ` Drew Adams 2012-04-26 15:35 ` Drew Adams 2012-04-26 18:38 ` Thierry Volpiatto 2012-04-24 18:54 ` Stefan Monnier 2014-02-09 4:32 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).