Ruurd Pels rightly pointed out that the temp variable cannot get longer than MAX_PATH+1 (MSDN says so) and that there is already a function in the newer Delphi RTL for getting this value: System.ioutils.TPath.GetTempPath. Looking at the code there (XE6) brings me to an interesting question:
// get memory for the buffer retaining the temp path (plus null-termination) SetLength(Result, MAX_PATH); Len := Winapi.Windows.GetTempPath(MAX_PATH, PChar(Result));
since the returned string can be up to MAX_PATH+1 characters long, including the terminating zero, is it save to do the above? Can we rely on the buffer of the string being one character longer than specified in SetLength?
Thomas Müller Also, your error checking is broken. GetTempPath returns 0 to indicate an error. You test for <0. Not going to happen. Not least because the return value is unsigned.
Thomas Müller Your GetTempPath is hugely overcomplicated. Here's a simpler version:
function TempPath: string; var Buffer: array [0..MAX_PATH] of Char; LastError: DWORD; begin if Windows.GetTempPath(Length(Buffer), Buffer)=0 then begin LastError := GetLastError; RaiseLastOSErrorEx(LastError, _('TFileSystem.GetTempPath: %1:s (code: %0:d) calling Windows.GetTempPath')); end; Result := Buffer; end;
David Heffernan The title said "Using the %Temp% Directory" and thats what the function above returns. If anything else is expected, the title/content of the article is wrong.
(The function above returns the same as doing %temp% in explorer)
Alexander Benikowski If you want to read the TEMP environment variable. Use GetEnvironmentVariable('TEMP'). If you want to find the location of the temp directory, use GetTempPath. The two are not the same. The blog article is badly titled.
GetTempPath doesn't say that there are multiple temp directories. What it gives is the algorithm used to find the temp directory. If there is a TMP var, that defines it. Otherwise, if there is a TEMP var, that defines it. And so on. That does not mean that there are multiple temp directories. For a specific user, there is one temp directory. Modulo the fact that different process have different environments etc. etc.
David Heffernan thanks, I must have missed the MAX_PATH reference in the API documentation when I originally wrote that function (several years ago). Also the fact that it returns 0 on error must have escaped me back then. I have already fixed the code and will update the post to reflect that later.
It's just general good practice to avoid heap allocations. For performance reasons. In my view. The error checking is semantically wrong too. The docs could not be clearer. If the function fails, it returns 0. So test for equality to 0.
why not use GUID for unique directory name?
ReplyDeleteYes, would have been an option. I don't like GUIDs much because users keep on trying to quote them to me over the phone.
ReplyDeletejust a thought, after all, it's in temp dir
ReplyDeleteRuurd Pels rightly pointed out that the temp variable cannot get longer than MAX_PATH+1 (MSDN says so) and that there is already a function in the newer Delphi RTL for getting this value: System.ioutils.TPath.GetTempPath.
ReplyDeleteLooking at the code there (XE6) brings me to an interesting question:
// get memory for the buffer retaining the temp path (plus null-termination)
SetLength(Result, MAX_PATH);
Len := Winapi.Windows.GetTempPath(MAX_PATH, PChar(Result));
since the returned string can be up to MAX_PATH+1 characters long, including the terminating zero, is it save to do the above? Can we rely on the buffer of the string being one character longer than specified in SetLength?
Thomas Müller the RTL code can indeed break.
ReplyDeleteThomas Müller Delphi strings (ansi and unicode) are also null terminated.
ReplyDelete"For Delphi programmers it is a bit cumbersome to use "
ReplyDeleteNope, that is what TPath.GetTempPath/GetTempFileName are for.
Stefan Glienke when you have a Delphi version providing TPath...
ReplyDeleteOr using JclFileUtils.PathGetTempPath()?
ReplyDeleteJeroen Wiert Pluimers is wrong. If N>0 and S is a long string variable, then SetLength(S, N) allocates N+1 characters.
ReplyDeleteThomas Müller Also, your error checking is broken. GetTempPath returns 0 to indicate an error. You test for <0. Not going to happen. Not least because the return value is unsigned.
ReplyDeleteThomas Müller Your GetTempPath is hugely overcomplicated. Here's a simpler version:
ReplyDeletefunction TempPath: string;
var
Buffer: array [0..MAX_PATH] of Char;
LastError: DWORD;
begin
if Windows.GetTempPath(Length(Buffer), Buffer)=0 then begin
LastError := GetLastError;
RaiseLastOSErrorEx(LastError, _('TFileSystem.GetTempPath: %1:s (code: %0:d) calling Windows.GetTempPath'));
end;
Result := Buffer;
end;
Note also that GetLastError returns a DWORD.
how about GetEnvironmentVariable('Temp')?
ReplyDeleteAlexander Benikowski That's not the temp directory though is it. Read the documentation of GetTempPath.
ReplyDeleteDavid Heffernan The title said "Using the %Temp% Directory" and thats what the function above returns. If anything else is expected, the title/content of the article is wrong.
ReplyDelete(The function above returns the same as doing %temp% in explorer)
David Heffernan oh and there are MULTIPLE temp-directorie. %Temp% is one of them(and noted in the documentation of
ReplyDeleteGetTempPath)
Alexander Benikowski If you want to read the TEMP environment variable. Use GetEnvironmentVariable('TEMP'). If you want to find the location of the temp directory, use GetTempPath. The two are not the same. The blog article is badly titled.
ReplyDeleteGetTempPath doesn't say that there are multiple temp directories. What it gives is the algorithm used to find the temp directory. If there is a TMP var, that defines it. Otherwise, if there is a TEMP var, that defines it. And so on. That does not mean that there are multiple temp directories. For a specific user, there is one temp directory. Modulo the fact that different process have different environments etc. etc.
well, %TEMP% contains one of the possible temp-paths^^
ReplyDeleteEdit: ofcourse, it might not exist.
David Heffernan thanks, I must have missed the MAX_PATH reference in the API documentation when I originally wrote that function (several years ago). Also the fact that it returns 0 on error must have escaped me back then. I have already fixed the code and will update the post to reflect that later.
ReplyDeleteThanks for the feedback everybody.
ReplyDeletePost updated.
ReplyDeleteHow can GetTempPath return a value less than zero? Why do two heap allocations when one suffices?
ReplyDeleteI don't understand what you mean with "two heap allocations". The SetLength(Result, ...) calls? How would one suffice?
ReplyDeleteUse a local buffer (i.e. on the stack) for the actual API call, and then a single heap alloc with SetString.
ReplyDeleteWhy is a buffer on the stack better than on the heap? We are not talking a function that is called many times in a tight loop here.
ReplyDeleteIt's just general good practice to avoid heap allocations. For performance reasons. In my view. The error checking is semantically wrong too. The docs could not be clearer. If the function fails, it returns 0. So test for equality to 0.
ReplyDeleteThomas Müller to prevent memory fragmentation.
ReplyDelete