This is an archive of the discontinued LLVM Phabricator instance.

Make File option flags consistent for Python API
ClosedPublic

Authored by fjricci on Mar 16 2016, 2:50 PM.

Details

Summary

Fixes SBCommandReturnObject::SetImmediateOutputFile() and
SBCommandReturnObject::SetImmediateOutputFile() for files opened
with "a" or "a+" by resolving inconsistencies between File and
our Python parsing of file objects.

Diff Detail

Event Timeline

fjricci updated this revision to Diff 50876.Mar 16 2016, 2:50 PM
fjricci retitled this revision from to Make File option flags consistent for Python API.
fjricci updated this object.
fjricci added subscribers: sas, lldb-commits.
clayborg requested changes to this revision.Mar 17 2016, 10:02 AM
clayborg edited edge metadata.
clayborg added inline comments.
source/Host/common/File.cpp
48

This change isn't needed if "File::eOpenOptionWrite" is specified in the "a+" spec below...

source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1258

Docs for this say:

``a+''  Open for reading and writing.  The file is created if it does not exist.  The stream is posi-tioned positioned
        tioned at the end of the file.  Subsequent writes to the file will always end up at the then
        current end of file, irrespective of any intervening fseek(3) or similar.

Why is the File:: eOpenOptionWrite not included for "a+"?

This revision now requires changes to proceed.Mar 17 2016, 10:02 AM

So this was definitely a decision that I was debating. But I assume that the Append flag must imply the Write flag, since you can't open a file for Append without also opening it for write ("a" and "a+" both include write privileges). So I figured that having both "Append" and "Write" was redundant. And since "a" was previously using "Append" while "a+" was previously using "Write", I think that one of them needed to be changed to be consistent with the other. I can do it the other way around though if that's preferable.

Just to be clear in the code we should set the write flag and not make any assumptions. So then the first diff isn't needed and only PythonDataObjects.cpp needs to change.

That makes sense. Will do.

fjricci updated this revision to Diff 51005.Mar 17 2016, 6:09 PM
fjricci edited edge metadata.

Always use write flag, even in append mode

clayborg requested changes to this revision.Mar 18 2016, 9:56 AM
clayborg edited edge metadata.

Looks great, thanks for catching and fixing this. Can we add a test for this so we don't regress?

This revision now requires changes to proceed.Mar 18 2016, 9:56 AM

I want to add this testing to functionalities/command_script_immediate_output/TestCommandScriptImmediateOutput.py, since that test is already very basic coverage of the same functionality.

However, it appears that TestCommandScriptImmediateOutput is an expected fail on FreeBSD, Linux, and Windows. And when I run it on the master branch on OSX, fails as ERROR. So it might be that the test doesn't actually pass on any platform.

When I run locally on Linux, the test succeeds, so I extended the test and made sure it passes Linux locally. But I'm not sure how useful that is, since the test is currently disabled on Linux.

I'll upload the test I wrote and we can go from there.

fjricci updated this revision to Diff 51113.Mar 19 2016, 8:29 AM
fjricci edited edge metadata.

Added unit test for python file api

Friendly ping.

clayborg accepted this revision.Mar 24 2016, 2:12 PM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Mar 24 2016, 2:12 PM
sas closed this revision.Mar 24 2016, 3:28 PM

Committed as r264351.