OP2Archive Application Development

[Vagabond]

I have the CREATE command working well now. The REMOVE command is now written but untested. I'll probably test the REMOVE command and then write the ADD command after that.

For REMOVE and ADD, I'm creating a temp folder labeled OP2ArchiveTemp-RANDOMNUMBER. Then temp storing the files in this folder while deleting the existing archive and creating the new archive with updated files. I'm using cstdlib with the rand function seeded to the current system time to create a random number. Then the temp directory is deleted.

The main menu music is a separate wav file. It's not packed in the CLM.

I feel a little dumb for not knowing that. So yeah, the CLM pack is working fine.


I actually own one of the books mentioned in the Stack Overflow discussion on filename/FileName, and I agree with the book's advice where it specifically discusses compound words. It does matter if one considers filename one word.

Cwalina, Krzysztof. Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries (2nd Edition) (Microsoft Windows Development Series) (Kindle Locations 1294-1301). Pearson Education. Kindle Edition.

3.1.3 Capitalizing Compound Words and Common Terms

Most compound terms are treated as single words for purposes of capitalization.   DO NOT capitalize each word in so-called closed-form compound words. These are compound words written as a single word, such as endpoint. For the purpose of casing guidelines, treat a closed-form compound word as a single word. Use a current dictionary to determine if a compound word is written in closed form. Table 3-2 shows capitalization for some of the most commonly used compound words and common terms.

It is a Microsoft/.Net heavy response, but this book is actually why I use filename over fileName. It is pretty much my goto for formatting my code variable and method names and other conventions in C#. The book is a pretty good read for trying to create a coherent large library of code. (Not that you would know it reading a lot of my code... Although that is exacerbated by my lack of experience in C++ and learning on the fly.)

filename
[fahyl-neym]

noun
1. an identifying name given to an electronically stored computer file, conforming to limitations imposed by the operating system, as in length or restricted choice of characters.

Based on the Random House Dictionary, © Random House, Inc. 2017.

In common English I would write "update the filename to read XYZ." I would not write "check the filesize." I also would also not write "change the foldername."

Ultimately though, it is more important to be consistent, so I'm ok switching all my code over to fileName if you much prefer it.

On a similar topic, I noticed you are capitalizing the first letter of your function names in the archive code. I was starting to use lowercase for the first letter to match the standard library. But now that I think about it, I'm refusing to use underscores everywhere, so maybe emulating the C++ standard library is actually a bad idea. I'm leaning towards going back and changing them all to uppercase first letter to match your archive code... I like the idea of consistency with the standard library, but maybe it is just too out of date with naming conventions to be worth emulating?

-Brett

[Hooman]

goto  =>  goTo   


I picked up the PascalCase (or UpperCamelCase) from Microsoft. That's how the Windows API functions are written. Some sources use camelCase (or lowerCamelCase) for function names. I'm ok with either. Just so long as it's not snake_case.

Indeed, I would say a lot of the C standard library naming conventions are terrible, and not worth emulating. Particularly the excssv abbrvtns. 

Quote
The main menu music is a separate wav file. It's not packed in the CLM.

I feel a little dumb for not knowing that. So yeah, the CLM pack is working fine.

I probably didn't realize that for years.


I've been tempted to work on this stuff a number of times. Though each time I open one of the projects, I quickly realize I have no way to compile or test small changes. Changing it to compile on Linux doesn't seem like such a small task either. To counter this, I've recently setup a Windows VM with a copy of Visual Studio. (That also wasn't a small easy task). Maybe I'll get to something soon.

[Vagabond]

Glad to hear you have the vm working. Looking forward to any changes you make to the archive library or the rest of the source code. Feel free to break it as much as you want.

I will be mostly out of pocket this weekend and the following week, but should be able to jump back in next weekend.

Did a little work today troubleshooting the REMOVE command. I can confirm it is pretty broken. I'll have to wait to get it working though. Will post code to repo shortly. Everything compiles. REMOVE and some parts of EXTRACT just don't work properly.

-Brett

[Vagabond]

I was working on the REMOVE Command. In the process, I stumbled on the fact that the syntax

Code: [Select]

for each ( auto i in v ) 

is actually Microsoft specific.

I was using it because the platform agnostic call is a bit awkward looking and not as intuitive:

Code: [Select]

for ( auto i : myints )

Anyways, it is an easy switch to move everything over to using the second approach. I'll try to get it switched over on Saturday before the pair programming.

When using the REMOVE command, an error is thrown when trying to delete the original file. I ran out of time for troubleshooting today though.

Looking forward to getting this project wrapped up. 

-Brett

[leeor_net]

Dafuq does Microsoft keep doing stuff like that? I hate when they add their own brand of cuteness into the language. It's almost like they want code to break.

Had I seen that line I would have questioned it immediately as it most definitely doesn't conform to C++11 standards. 

[Vagabond]

Unfortunately, the pair programming wasn't possible due to some internet bandwidth issues. However, we did manage to discuss some issues over IRC and make some progress.

A small change was made to the clmFile class to allow its destructor to work when clmFile is being referenced by its base class, ArchiveFile.

The REMOVE command is now complete and tested. Next up is the ADD command.

Hooman spent some time removing the Windows.h code from the ClmFile, although there is still more to go.

Leeor managed to port OP2Archive and OP2Utility to GitHub. You can find them here: https://github.com/OutpostUniverse.

-Brett

[Hooman]

for each ( auto i in v )

I wouldn't have even noticed that. Particularly not if it was buried in a project which I assume already compiles and works. It looks too much like other languages, and language extensions.


As for the virtual destructor thing, I learned a thing or two about C++ these last couple of days when I tried making a destructor pure virtual. It seems pure virtual functions can have implementations, and in the case of destructors, the implementation is required. Further, you can't provide an implementation in the declaration that marks the destructor as pure virtual. You can however implement it later in the header after the close of the class, and mark it inline to avoid violating the One Definition Rule (ODR) should the header be included by more than one compilation unit. Setting it "= default" for the implementation works just fine, if that's what you want.

Other strange corner cases I found myself exploring, do abstract base classes have virtual function tables? What about classes with inherited virtual methods, which don't override any? And on a related note, both to this problem, and Microsoft creating their own extensions, there is the __declspec(novtable) extension. All very fascinating topics.

So many years of using C++, yet I'm still surprised by all the corner cases.

[Vagabond]

So many years of using C++, yet I'm still surprised by all the corner cases.

I'm learning C++ is not a language that one ever masters all uses and corner cases for... (at least I probably never will).

I once read a book by Douglas Crockford called JavaScript, The Good Parts. In part of it he said something to the effect that a good programmer should learn what each programming language is designed to do and attempt to use each language as designed. They should avoid oddball or corner case features in a language whenever possible. This increases readability for others, reduces the chance of exposing underlying bugs in the framework or language, and reduces the chance of using a feature that will become deprecated in the future. This always stuck with me as being a good practice.


I'm up and running just fine with GitHub. For cases where I'm developing by myself and just committing common changes, it is pretty much the same as my experience with HG and Bitbucket.

I've been cleaning the code a fair amount. The main.cpp function had approached 500 lines. It is now back down to about 170. Each major COMMAND (ADD, REMOVE, EXTRACT, etc) is now separated into a different subclass. Not much reason for the subclasses except encapsulating the code so only the required one or two functions to execute the class is visible to the rest of the project and not all the helper functions.

I've just started the ADD command, but it will need a lot of work. The time I can put in the project in the coming week may be fairly erratic.


Hooman, if you are planning to significantly alter the way the class ArchiveFile works, please let me know so I can plan accordingly. I'm thinking that you are just pulling out the Windows specific portions from VolFile, ClmFile, and MemoryMapFile and not changing how it works externally but we haven't really directly stated this. Not a big deal if you are changing, it just may mean that I should wait some before really polishing my portion if a lot of changes will need to be made.

-Brett

[Hooman]

Hmm, good point. I was planning mostly implementation changes, with hopefully little impact on the interface. At least that's how I see changes to remove dependence on windows.h. There are other areas I might like to update though.

One thing on my mind is the idea of using string and vector rather than char* and array. That could lead to some interface changes. I suspect the impact on your code would be minimal, since you're already using string and vector. Just remove the conversions before the calls.

I also had thoughts of reworking the class hierarchy to remove multiple inheritance. Perhaps make some methods static. It seems you've been working around problems related to that, so it could simplify some of your code. I'm not really looking at this idea yet though.


I took a look at your updates. Seems you've done a lot to simplify the command line argument processing.

I noticed in selectCommand, you have a bit long switch statement. I think that can be simplified. A switch statement is basically a jump through a jump table, and in your example each jump destination is basically just a call to some method. This is similar to virtual function dispatch, which is a call through a table of pointers to functions. You could potentially replace the switch with a call that uses virtual function dispatch. The result should be more compact and easier to maintain. It gets rid of the tedious process of adding to the switch statement when you add new commands. I hardly ever use switch anymore, now that I understand virtual functions.

To make it work, define a common base class, perhaps named ConsoleCommand. It would have derived classes List, Find, Extract, Create, Help, Add, Remove. The base class would contain a pure virtual method Execute(const ConsoleArgs& consoleArgs). The common Execute name for each command means your cases all collapse down to the same call, which is dispatched virtually based on the class type of the underlying command.

Code: [Select]

command->Execute(consoleArgs);

The part I haven't addressed yet, is how command is set to an object of the appropriate type. There are many possible solutions there. I'll take a further look at your code before suggesting something there. It seems you're mapping commands to an enum index. Where that is done could be an appropriate tie in point.

[Vagabond]

One thing on my mind is the idea of using string and vector rather than char* and array. That could lead to some interface changes. I suspect the impact on your code would be minimal, since you're already using string and vector. Just remove the conversions before the calls.

I also had thoughts of reworking the class hierarchy to remove multiple inheritance. Perhaps make some methods static. It seems you've been working around problems related to that, so it could simplify some of your code. I'm not really looking at this idea yet though.

Switching to std::string and or vector would be an easy fix on my side. Adding static create functions could simply my CREATE, ADD, and REMOVE commands by not requiring an initial .VOL and .CLM template file, which would be nice.

However, I've found the archive code pretty stable and easy to work with, so I'm fairly happy if it just ends up becoming platform agnostic without the rest.

I'll take a look at command->Execute() in the near future.

You are correct about mapping the COMMAND into an enum. This is done in ConsoleArgumentParser.h/.cpp. It is supposed to take the char** command line input and put it into an easier to understand structure (ConsoleArgs). Since ConsoleArgs is used by all the available commands, there is still some messiness associated outside of ConsoleArgumentParser. For example, how the member variable paths is used will depend on which COMMAND is being used. In CREATE, only the first path should by a .vol/.clm where in EXTRACT several .vol/.clm paths could be provided. Also, some optional arguments do not apply to all possible commands. Like -Q / --Quiet and -O / --Overwrite doesn't do anything for the LIST command.

I could pass more customized structures out of ConsoleArgumentParser that exactly lined up with each command, but this would force me to have multiple custom structures instead of using one general structure. The generalized structure seems to be generally working fine for now, but I wonder if it might be a little clearer to make the specialized structures (sorry for the bad pun).

I could not encapsulate ConsoleArgumentParser.h/.cpp into a class. I'm using the #include <functional> to allocate a delegate function based on which command argument is being parsed. When I pushed the code into a class, it seemed to always break the struct containing the functional delegate. (I call them delegates in C#, I don't think they are 100% the same in C++ though). Anyways, I just encapsulated ConsoleArgumentParser.h/.cpp in a namespace instead and everything is working. This is something I'd like to return to and clean up at some point though.


I finished a working draft of the ADD command. While dealing with some memory management in the ADD command class, the purpose of the RAII paradigm suddenly clicked. Basically, I needed to ensure that 2 objects were destroyed whether an exception is thrown or not. So I abstracted out their destruction into a separate function. Then I placed the function call cleanup() in the catch block and re-throw the exception and place the function call at the end of the task for when no exception occurred.

Then it occurred to me that I should probably be destroying the object in the class's destructor and not be adding all the random cleanup() function calls. Unfortunately I designed the class to not have any class level member variables, so RAII doesn't really work yet. But it might make sense to move the pointers to class level variables to leverage RAII. I'll have to consider the design implications.

Hopefully that makes some sense. If not, I'm still learning and probably am not explaining things very well.

-Brett

[Vagabond]

Another quick update.

I have all the commands working as advertised except for the EXTRACT command which needs a couple of tweaks. A couple of tasks left to finish besides refactoring EXTRACT is pushing in use of unique_ptr over the raw pointer, building up the post build event, and finalizing the Usage Statement/ReadMe. I also have some more general cleaning of the code that I'd like to accomplish.

-Brett

[Vagabond]

Hey everyone, we are on the home stretch for completing OP2Archive for Windows.

I finally rewrote the EXTRACT command code into something that makes reasonable sense and is cleaner to understand.

I finished a post build event for RELEASE compilation mode that automatically creates a zip file containing the executable, 2 template archive files, and the readme with the version number included in the zip name.

I created a draft page on the wiki to post OP2Archive at. It will still need a bit more work.

I completed another pass on ReadMe.txt and finalized the command list. I separated out information on how to compile and distribute the software into a separate file called CompileInstructions.txt. I did this because the average user isn't interested in compiling, but just how to use the application. ReadMe.txt points to CompileInstructions.txt for anyone interested in examining the source code.

If someone could read through the ReadMe.txt and CompileInstructions.txt below and let me know if they see any grammatical errors or omissions, it would be helpful. The first dozen lines are the usage statement that will be displayed in the command prompt if you type -h/-?, so let me know if it seems like a reasonable usage statement as well.

Next up is final refactoring and testing of the code.

-Brett


ReadMe.txt
OP2Archive - Outpost 2 Archive Access and Maintenance
Developed by Hooman and Brett208 (Vagabond)

Allows examining, creating, and extracting files from Outpost 2 .vol and .clm archives.

+++ COMMANDS +++
  * OP2Archive LIST (archivename.(vol|clm) | directory)...
    * Lists the contents and size of all files contained in provided archives.

  * OP2Archive FIND filename...
    * Determines which archive contains the file. Stops searching at first instance.
    * Pulls archives from the directory appended to the provided filename to search.

  * OP2Archive CREATE archivename.(vol|clm) [filename | directory]... [-q] [-o]
    * If no filename(s) or directory(s) provided,
      archives all contents of the default source directory (./archiveFilename).

  * OP2Archive EXTRACT archivename.(vol|clm) [filename]... [-q] [-d destDirectory] [-o]
    * If no filename(s) provided, extracts entire contents of archive.

  * OP2Archive EXTRACT directoryname [-q] [-d destDirectory] [-o].
    * Extracts entire contents of all archives in the provided directory.

  * OP2Archive ADD archiveName.[vol|clm] filename... [-q] [-o]

  * OP2Archive REMOVE archiveName.[vol|clm] filename... [-q] [-o]

+++ OPTIONAL ARGUMENTS +++
  -H / --Help / -?: Displays help information.
  -Q / --Quiet: [Default false] Prevents application from issuing console messages.
  -O / --Overwrite: [Default false] Allows application to overwrite existing files.
  -D / --DestinationDirectory: [Default is './']. Sets the destination directory for extracted file(s).
  -S / --SourceDirectory: CREATE: [Deafault is archive's filename]. Sets the source directory when creating an archive.

For more information about Outpost 2 visit the Outpost Universe (http://outpost2.net/).


+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +


+++ INSTALL DIRECTIONS +++

Unzip OP2Archive.exe using WINZIP compatible decompression algorithm. The typical place to put OP2Archive.exe is in the root OP2 install directory for ease of use, but this is not required.

Open a command prompt. Navigate to the directory containing OP2Archive and type 'OP2Archive -h' to see Usage Message.


+++ ARCHIVE (.VOL/.CLM) FILE CREATION BEST PRACTICES +++

If you wish to overwrite a file currently in an archive file, use the ADD command and include the optional argument -O / --Overwrite to allow overwriting the original file.

The ADD and REMOVE command will create a new temp directory with the name ./OP2ArchiveTemp-(RANDOM INT) to store the contents of the archive file while rebuilding it. The ADD and REMOVE command will also eventually delete the original archive file before rebuilding it. If certain fatal exceptions occur during this process such as a power loss, the original archive file may be lost and/or the temp directory may not be deleted. If you are performing ADD and REMOVE commands on a heavily modifyied archive file, consider keeping a backup of the file somewhere. In case of a fatal error in the critical steps for the ADD and REMOVE coomands, you should be able to just recover the archive file from the Outpost Universe website by redownloading the game.


+++ CLM FILES (AUDIO STORAGE) +++

Outpost 2 stores all music tracks except for the track that plays on the main menu in the archive file op2.clm.

If you wish to change out the sound tracks in Outpost 2, you must use specific settings and use the WAV file format.

As a starting point for manipulating audio tracks for Outpost 2, consider starting with the free program Audacity (http://www.audacityteam.org/).

When naming audio files for storage in a CLM archive, the filename will be clipped down to 8 characters. Without siginificant modifications to the Outpost 2 application, you must use the names of the audio tracks provided with the stock download of the game to get modified or new tracks to play in game. The game will then select the music tracks as it sees appropriate based on name. So, the Eden tracks will play for Eden missions, etc.

Outpost 2 audio tracks must be formatted as WAV files with the following settings:
 * Frequency = 22050 Hz
 * Channels = 1 (Mono)
 * BitsPerSample = 16

The file size of each WAV audio track (the actual wav data) must be a multiple of 32768 bytes (0x8000). Output 2 fills the music buffer in chunks that large, and doesn't zero fill if a chunk (at the end of a file) is too short. If the file is not a multiple of 32768 bytes (0x8000), some garbage data will be played in Outpost 2 before switches to the next track. The audio data must be zero padded to bring it up to the right size before adding to the .clm file.


+++ COMPRESSION +++

Outpost 2 contains references to 3 types of compression, RLE (Run - Length Encoded), LZ (Lempel - Ziv), and LZH (Lempel - Ziv, with adaptive Huffman encoding).

Only LZH was used in the final release of Outpost 2. Only one archive file was compressed, sheets.vol. In subsequent releases of Outpost 2 by the Outpost Universe community, sheets.vol was decompressed and included in the game download in uncompressed format.

OP2Archive is capable of reading and decompressing archives using LZH compression. However, it currently cannot CREATE archives or modify via the ADD or REMOVE file an archive using LZH compression.


+++ SOURCE CODE LOCATION AND COMPILIATION +++

Source code may be found at: https://github.com/OutpostUniverse/OP2Archive. See the file CompileInstructions.txt in the source code for specific instructions.


+++ CHANGE LOG +++

Ver 1.0.0 (XXSep2017)
 * Initial Release



CompileInstructions.txt
OP2Archive Compilation Instructions
Developed by Hooman and Brett208 (Vagabond)

If you wish to edit and compile OP2Archive's source code, read this document first. See ReadMe for basic description of project, change log, and instructions on use once compiled.

Source code may be found at: https://github.com/OutpostUniverse/OP2Archive and https://github.com/OutpostUniverse/OP2Utility.

OP2Archive is written in C++ and the solution/project files are built for Visual Studio 2017. C++11 and C++14 features are used. The currently expiremental C++ standard library filesystem is used and is supported by both VS2017 and GCC.

OP2Archive depends on the project OP2Utility. OP2Utility requires a Windows machine to compile due to use of windows.h. In particular, the archive read/write code depends on Windows.h. There is some desire to remove the Windows specific code, but the efforts are not yet completed. The project must be compiled for x86.

+++ RELEASE COMPILATION INSTRUCTIONS +++

Post Build Event Notes: When in RELEASE mode, a command prompt script will run once the source code is compiled. If compiling for test purposes, compile in DEBUG mode to prevent the script from running. This script automates staging all required files for distribution. Included in the source code is 7za.exe, the 7-zip command line tool that facilitates zipping the final release package. This is the Windows version of 7 zip and is not compatible with Linux. See http://www.7-zip.org/ for source files and https://www.dotnetperls.com/7-zip-examples for examples of use.

 1. If changing Major/Minor revision number, set new version number at top of OP2Archive main.cpp AND in post build event batch script.
 2. Run Commit and then push Updates to master repository.
 3. Set Solution Configuration to Release.
 4. Set Solution Platform to x86. (x64 is not currently supported due to some WINAPI code within OP2Utility VolDecompress).
 5. Compile Code.
 6. The following files will automatically be copied into the zipped directory'OP2MapImager 1.0.XXXX' (XXXX is svn revision number):
    * OP2Archive.exe (From Release Directory)
   * VolTemplate.vol
   * ClmTemplate.clm
   * ReadMe.txt (this file)
 7. Place zip file on the Outpost Universe Website.

[Hooman]

Hey, good to see this released.

In the ReadMe.txt, I found this one sentence a bit unclear:

    * Pulls archives from the directory appended to the provided filename to search.

As for the credits, you should probably put your name first on this one.

[Vagabond]

I just finished testing the different modes of OP2Archive. It seems to be working properly. I tested it both using the current directory and manipulating files on relative paths.

Several bugs were found and squashed and the code further refactored.

I'm starting to see the value of Unit Tests! Every time I make a change to OP2Utility, it affects both OP2MapImager and OP2Archive, both of which are now mature, stable projects (although small). It would nice to just open each project and push a button to make sure the change didn't blow something up in either project!

There is definitely room for improvements in the source code, and introduction of Git submodules (I think???) for more proper linking of OP2Utility. For now though, I want to call it done besides fixing bugs if anyone finds them while using it.

Thanks for the help from Hooman, Arklon, Leeor, and sirbomber answering my random Outpost 2 or C++ questions working through this project and other projects!

I'll push the actual release in a separate thread and host it on a Wiki page.

In the ReadMe.txt, I found this one sentence a bit unclear:

Yeah, that is pretty bad wording. I reworded it in the release usage statement to try and make it more clear. If people are using the FIND command and would prefer the directory to search for archives in be an argument/switch instead of appended to the filename being searched for, we could try that instead.

-Brett

[Hooman]

We should totally look into both Git submodules and unit tests. I think the Git submodule thing should be easy enough. Unit tests could take a bit of time to write a meaningful number of. Lately I've often been finding myself curious about the Google unit test library that was mentioned here a while back.

[Vagabond]

I'm getting hung up on a couple of bugs during the final testing. I've fixed one myself, but there was another related to exceptions that I wanted to mention.

The ClmFile constructor contains a couple of throws for exceptions when trying to access and parse a clm file. These throws are followed by a char* statement only and not an exception class. Like this:

Code: [Select]

ClmFile::ClmFile(const char *fileName) : ArchiveFile(fileName)
{
	... code to open a file handle ...

	if (m_FileHandle == INVALID_HANDLE_VALUE) 
		throw "Could not open file";
		
	if (ReadHeader() == false) 
		throw "Invalid header";

	... more code ...
}

When these exceptions are triggered, even though I wrapped the call to this function in a try catch statement, it blows up the application.

Below I rewrote the function to throw an actual exception as opposed to just a char*. This now makes it behave properly and allows my try/catch to catch the exception and spit out a message to the user about the malformed file.

Code: [Select]

ClmFile::ClmFile(const char *fileName) : ArchiveFile(fileName)
{
	... code to open a file handle ...

	if (m_FileHandle == INVALID_HANDLE_VALUE) 
		throw exception("Could not open file");
		
	if (ReadHeader() == false) 
		throw exception("Invalid header");

	... more code ...
}

Since C++ allowed the code to compile and operate by throwing the char* without including the exception class, I assume there must be a time/place to do this?

So, when would I want to throw char* instead of an exception? Is it fine with everyone if I just throw an exception instead of the char* so that I can properly catch the exception?

VolFile.cpp and possibly other code in the archive project have similar throw statements. I would probably change them all out if this sounds agreeable.

Someday I'll stop procrastinating on the release of this project, I promise.

Thanks,
Brett

[Hooman]

Someday I'll stop procrastinating on the release of this project, I promise.

Ahahahaha! Now there's something every programmer can relate to 


Yes, you can make this change, and yes it would be a good idea. Though I would recommend using std::runtime_error instead of std::exception. The reason being the standard does not define a constructor which takes a string for std::exception. This is a Microsoft extension. Code that tries to pass a string to an std::exception constructor will fail on Linux. Note that std::runtime_error derives from std::exception, so you can still catch by a reference to an std::exception.

As for why the code was written the way it was. Simply, I was fairly new to using exceptions at the time. In C++ you can throw and catch anything you want. They don't have to be derived from std::exception. Though if you throw something that is not derived from std::exception, you can not catch by reference to an std::exception object. Hence why you're likely not catching the exception. If instead you'd caught a char*, it would have worked.

Note that a try block can have multiple catch blocks, each with a different data type that is caught. You could have one catch block that catches char*, while the next catch block catches a std::exception&. Ideally though, exceptions should be derived from std::exception, and not just some arbitrary datatype.


Throw runtime_error everywhere and it should be good.

[leeor_net]

I very seriously doubt a catch(char*) will work. Probably need a catch(const char*) but even then I suspect there's an implicit conversion going on. You should either catch(const std::string&) OR throw std::runtime_error(const std::string& what).

Most of the time I use std::runtime_error but in some cases it makes sense to derive 'specialized' exception objects from runtime_error:

Code: [Select]

class SomeExceptionClass : public std::runtime_error
{
public:
    SomeExceptionClass() : std::runtime_error("Message Here") {}
    virtual ~SomeExceptionClass() {}
};

I use this in NAS2D for very specific errors that the user can recover from. For example:

Code: [Select]

class filesystem_already_initialized;
class font_bad_data;
class image_bad_copy;
class mixer_backend_init_failure;
class renderer_opengl_context_failure;

And etc. This allows the program to catch a specific error or provide a generic std::runtime_error& handler:

Code: [Select]

NAS2D::Image image1("path/to/image.png");

try
{
    NAS2D::Image image2 = image1;

    NAS2D::Font font("path/to/font.ttf");
    /* exception of type font_bad_data thrown here */
}
catch(const image_bad_copy& e)
{
    /* Some sort of exception handling in the case of image_bad_copy */
    std::cout << "Failed to copy image: " << e.what() << std::endl;
    /* Possibly set up a default image of some sort here to visually indicate an error */
}
catch(const std::runtime_error& e)
{
    /* No handler for font_bad_data, so catch by reference to base type as a generic handler */
    std::cout << "Some other runtime error that doesn't require exploding: " << e.what() << std::endl;
}

Note that the above code doesn't attempt to catch possible other exception types, e.g., a std::bad_alloc. In a case like a failed memory allocation it would be appropriate in many cases to allow the program to unwind back to the original try block and possibly to just exit alltogether.

[Hooman]

You're quite right, it would need to be catch(const char*). I suspect this is a somewhat recent change in the language where embedded text strings no longer implicitly convert to char* and you're forced to use const char* instead. That means my old code is broken now, since it was indeed catching char*.

Not too sure what you meant by suspecting there was an implicit conversion going on. Should be no conversion for const char*, since an embedded text string will be of type const char*. I tried catching with a string& and a const string&, but neither would work.

Example:

Code: [Select]

#include <iostream>
#include <string>

using namespace std;

int main()
{
  try {
    throw "A text string";
  }
  catch(char *) {
    cout << "char*" << endl;
  }
  catch(const char*) {
    cout << "const char*" << endl;
  }
  catch(const string&) {
    cout << "string&" << endl;
  }

  return 0;
}

Output:

Code: [Select]

const char*

If I remove the catch(const char*) block, leaving the others, the output is:

Code: [Select]

Aborted (core dumped)

Good point on the double catch design. That is useful to handle error of different severity, or that require different logging output.

[Vagabond]

Leeor and Hooman,

Thanks for the input. I went ahead and switched all my throws from exception to runtime_error or logic_error or invalid_argument as I saw fit. I also included <stdexcept> in all the .cpp files that throw exceptions. Turns out I hadn't included <stdexcept> or <exception> in any of the .cpp files, but probably dragged them into my code by including <string> and <vector> everywhere.

I also appended runtime_error to all the throws in the archive code that was throwing char*.

Interesting to know that you can throw anything you want. It didn't occur to me to do something like this though.


On another note, I've been thinking more about namespaces. Just like I brought in exceptions to my code without explicity including them, the same can happen with namespaces.

Code: [Select]

using namespace std;

I've been putting this in a lot of my header files to keep from typing std:: everywhere in my code just to use a string.

I'm realizing this forces everyone else to merge the entire std library in their code if they ever include one of my headers. Not good.

I'm going to move all using statements out of header files and below any includes. This way it is a private choice of the .cpp file and doesn't force the std namespace on anyone else.

I think moving using statements out of header files and below all includes is universally seen as a right practice. There seems to be a lot of debate whether it is okay to bring std into the global namespace of a .cpp file. One camp says it could cause future naming collisions when the std library is expanded or you add another include to your code. The other camp says it makes the code in the .cpp file harder to read and bloats your code by typing std:: everyone.

I haven't tried it yet, but I would hope the compiler would flag name collisions for me so I can sort out which function I mean to call by explicitly tagging it with its namespace.

Either way I think they both have reasonable points. For now I'm going to keep the using statements in the .cpp files for clarity and ease of typing. The important thing is for them to be out of the header files so I don't force the std library into everyone else's global namespace.

Technical bulletin #435 dealing with the oddities of C++, understanding the implications of include and using statements in modern C++ code.

So one more thing to work through in OP2Archive before it is released. I'm also learning more how TortoiseGit handles submodules, but not enough to write intelligently about it and probably not something I want to pollute this thread with.

-Brett

[Hooman]

Ahh, very good call on the using namespace std; thing.

I used to really hate typing std:: everywhere, so I always used using. It does have issues in header files though. I've also come across a few articles that suggested never using using. One mentioned people being bothered by it at first, but quickly getting used to it, and eventually liking the more verbose and precise code. I've been giving it a try. I'm now at a point where the extra typing bothers me much less. I still have a lot of forgetful moments that the compiler catches for me. I figure I'll continue with this style of not using using, and instead typing std:: everywhere.

So, any other users still musing about using using for the sake of the users using your using-free code, or is your using-free code still free of users, leaving you usually unused to using criticism?   


Will be looking forward to anything you write about Git submodules. I still need to learn that better. I've seen command line example that largely make sense now, but I don't know how Git tracks it all under the hood, or how to do these operations with TortoiseGit.

[leeor_net]

I tend to go with explicitly stating std::whatever in headers and using namespace std; in definitions (e.g., cpp codes) if it's a giant file. Lately I've been finding I prefer to explicitly state where things are being pulled from -- sure it's 'more typing' but in the event you have a library that pollutes the namespace you could potentially end up with annoying errors related to ambiguity. Explicitly naming the namespace avoids this though ultimately it's a preference.

I think it's safe to say that best practices require explicitly naming namespaces in headers and allowing the end user to decide if they want to use a using directive.

[Hooman]

I'd also like to point out that typing speed is never the limiting factor for programming speed. Plus with IDEs and autocomplete, you can get away withReallyReallyLongIdentifiersSuchAsInLanguagesLikeJava, and it won't slow you down very much. Just hope you have a widescreen monitor.

There are some interesting stats thrown about concerning the average number of lines of code a developer writes in a day. Typical figures are about 10 lines. Granted, people can write a few thousand lines of code in a day, but then might refactor, and remove a few hundred to a thousand lines the next day. Then they might spend a few days thinking about a problem, perhaps reading and testing, or reviewing other people's code, and not actually write any of their own. They might also need to fix a bug in someone else's code, which requires significant amounts of time to read and understand it before they can fix it. When all is said and done, the actually number of lines of code added per developer per day is typically much lower than most people assume. It's also a horrible metric for developer productivity, for many of the reasons just stated.

My point today is that, if we wish to count lines of code, we should not regard them as "lines produced" but as "lines spent": the current conventional wisdom is so foolish as to book that count on the wrong side of the ledger.

All this is to say, a little extra typing probably won't affect productivity too much. At least not if it doesn't add to mental complexity, or some kind of anguish over working on the problem that makes you want to not do it.

[Vagabond]

I will be releasing version 1.0.0 shortly. The recent fixes include:

* Listing the name of the file when a .clm or .vol file fails to load.
* Fixed bug not allowing -D / --DestDirectory to work properly
* Removed all namespace using statements from header files.
* Kept EXTRACT command from overwriting files unless given permission with the -O / --Overwrite switch.
* Removed -S / --SourceDirectory switch from the ReadMe.txt since it was removed from the application.

See: https://forum.outpost2.net/index.php/topic,6032.0.html

-Brett

[Hooman]

Awesome job